Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to TypeScript and React 16+ #549

Merged
merged 34 commits into from
Mar 29, 2020
Merged

Port to TypeScript and React 16+ #549

merged 34 commits into from
Mar 29, 2020

Conversation

zenoamaro
Copy link
Owner

@zenoamaro zenoamaro commented Nov 16, 2019

This updates the whole of ReactQuill to Typescript and React 16, carries with it a new build system, and several clean-ups.

For the vast majority of people, this should be a drop-in update, requiring no migration work. As a general rule, I would consider any such changes as bugs, and waive them if really impossible to achieve.

Mixin and Toolbar have been removed, as they have long been deprecated. Toolbar has been superseded by the Quill Toolbar Module, but no migration path is provided for Mixin. Hopefully we can cover any need for the Mixin with specific features.

The technique used to re-render the main component (regeneration) has been changed a bit, due to the changes required by the new lifecycle methods. The final behavior seems to be the same. I tested all the props that cause it, though not at extreme lengths.

I got rid of the Makefile-based build system (kind-of Windows unfriendly, also rarely found on JS developers' machines), in favor of just using package.json scripts. I switched to prepublishOnly to build only before publishing a package; lib and dist will be included in the package, so users should be fine. I will additionally switch rm -rf to rimraf so we can be fully windows-compatible.

I also got rid of hinting: I find Typescript's linting to be both more concise and more bearable, and yet covering everything that's actually important.

I think I'd leave a hooks rewrite for a follow-up ticket, as we risk altering the behavior too much. It's not strictly required to rewrite using hooks, as classes are not a bad practice or anything, but there is a perf benefit to using hooks, at the cost of a pretty long function component.

  • Support for React 16
  • Rewrite in TypeScript
  • Deprecate Toolbar and Mixin
  • Ensure behavior stays the same, no regressions
  • Refactor the build system
  • Make sure CommonJS imports work as they did in the past (if possible)
  • Double-check documentation for outdated or plain incorrect information
  • Publish 2.0.0-beta1 to NPM
  • Announcement and invitation to test beta on master
  • Wake up tickets that might be fixed by the beta
  • Release v2.0.0
  • Update GH pages
  • Update and move most code examples to Codepen
  • Call out to companies or products using ReactQuill

@zenoamaro zenoamaro self-assigned this Nov 16, 2019
@alexkrolick
Copy link
Collaborator

Oh dang 😃. I'll look at this later

package.json Outdated Show resolved Hide resolved
src/component.tsx Outdated Show resolved Hide resolved
src/component.tsx Outdated Show resolved Hide resolved
@dreyks
Copy link

dreyks commented Dec 1, 2019

that's a great chunk of work 👍
I'd definitely use the beta as soon as it's released

@ThePaulMcBride
Copy link
Contributor

Oh man, this looks good!

@FrederickEngelhardt
Copy link

👏

@moonrailgun
Copy link

Typescript is a good language!
As typescript user, imperfect type definition tortured me so much.

Hopes its work could be push. Thanks :)

@justinbhopper
Copy link

Looking forward to this beta release! If it goes beta, will there be a publish to the unpkg CDN too?

@gurkerl83
Copy link

It would be great to see the reimplementation in the near future in the form of a beta version. Is there any time frame when this will happen?

@alexkrolick
Copy link
Collaborator

alexkrolick commented Feb 17, 2020

@zenoamaro I'd like to go ahead and remove the deprecated exports. That will simplify the API and docs and make it easier to maintain the same or better API in a hooks-based version later.

harunurhan added a commit to harunurhan/inspirehep that referenced this pull request Mar 5, 2020
* upgrades to antd to v4
 * fixes UX problems with date range picker: INSPIR-3203
 * reduces bundle size by removing unnecessary icons: INSPIR-3228
* upgrades to react@16.13.10
* upgrades to react-router to v5

Unresolved:

* `componentWillReceiveProps` warnings from `react-vis`
uber/react-vis#1253

* search name space truncates some options like  `conferen...`
ant-design/ant-design#21754

* `componentWillMount` warnings from `react-helmet`
nfl/react-helmet#413

* `componentWillMount` warnings from `react-loadable`
jamiebuilds/react-loadable#220

* `Cannot update a component from inside the function body of a
different component.` worning from `antd`
ant-design/ant-design#21800

* `componentWillReceiveProps` warnings from `react-quill`
zenoamaro/react-quill#549
harunurhan added a commit to harunurhan/inspirehep that referenced this pull request Mar 5, 2020
* upgrades to antd to v4
 * fixes UX problems with date range picker: INSPIR-3203
 * reduces bundle size by removing unnecessary icons: INSPIR-3228
* upgrades to react@16.13.10
* upgrades to react-router to v5

Unresolved:

* `componentWillReceiveProps` warnings from `react-vis`
uber/react-vis#1253

* search name space truncates some options like  `conferen...`
ant-design/ant-design#21754

* `componentWillMount` warnings from `react-helmet`
nfl/react-helmet#413

* `componentWillMount` warnings from `react-loadable`
jamiebuilds/react-loadable#220

* `Cannot update a component from inside the function body of a
different component.` worning from `antd`
ant-design/ant-design#21800

* `componentWillReceiveProps` warnings from `react-quill`
zenoamaro/react-quill#549
harunurhan added a commit to inspirehep/inspirehep that referenced this pull request Mar 5, 2020
* upgrades to antd to v4
 * fixes UX problems with date range picker: INSPIR-3203
 * reduces bundle size by removing unnecessary icons: INSPIR-3228
* upgrades to react@16.13.10
* upgrades to react-router to v5

Unresolved:

* `componentWillReceiveProps` warnings from `react-vis`
uber/react-vis#1253

* search name space truncates some options like  `conferen...`
ant-design/ant-design#21754

* `componentWillMount` warnings from `react-helmet`
nfl/react-helmet#413

* `componentWillMount` warnings from `react-loadable`
jamiebuilds/react-loadable#220

* `Cannot update a component from inside the function body of a
different component.` worning from `antd`
ant-design/ant-design#21800

* `componentWillReceiveProps` warnings from `react-quill`
zenoamaro/react-quill#549
@ValentinH
Copy link

@zenoamaro @alexkrolick what is the current status of this PR? What steps are left for it to be merged?

Could I help in any way? 🙂

@cmrd-senya
Copy link

Great job! Hope to see it merged soon! @zenoamaro please tell us if there is a way to contribute to your work to make it delivered faster.

@zenoamaro
Copy link
Owner Author

@alexkrolick I took care of deprecated props and exports, and performed an initial pass over the documentation, adding a migration guide among other things. Can you do a second pass, and figure out whether I have missed something?

In the process, I also noticed that the Quill namespace wasn't being re-exported (which should be fixed now) but also: it's possible that CommonJS imports require require().default now. I will investigate.

I want to achieve a few things for this beta:

  • Make sure CommonJS imports work as they did in the past (if possible)
  • Find and update any outdated information in the README
  • An encouragement to test the beta, and hopefully iron out any remaining kinks
  • Wake up any ticket that may be fixed by this rewrite, and ask for reproduction

After the beta:

  • Move the many code examples to Codepen, to simplify editing the README
  • Figure out which companies or products are using ReactQuill in the wild, and have them listed in the README

@zenoamaro
Copy link
Owner Author

zenoamaro commented Mar 28, 2020

I have fixed the require('react-quill').default issue by re-exporting all the ES exports with module.exports. This seems to work well in all my test cases, including UMD.

Edit: after further investigation, while the exports themselves work fine, TypeScript's JS checking and VSCode's Intellisense can't deal with it. I resorted to the funky export = syntax as seen in microsoft/TypeScript#2719 (comment)

test/index.js Outdated Show resolved Hide resolved
* This can't be tested with the current state of JSDOM.
* The selection functions have been shimmed in this test suite,
* but they will not work until DOM traversal is implemented in
* This can't be tested with the current state of JSDOM.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsdom/jsdom#2719

this might actually work now, worth taking a look

@zenoamaro zenoamaro merged commit 6a0a2ee into master Mar 29, 2020
@zenoamaro zenoamaro deleted the typescript-rewrite branch April 15, 2020 18:52
@lukevp
Copy link

lukevp commented May 6, 2020

I'm a bit confused about the state of hooks support. Does this beta version support being used in a hook component (for example, with the value being rendered controlled like this?

export function Editor(props: IEditorProps) {
  const [editorValue, setEditorValue] = useState('');

  const handleChange = (content: any, delta: any, source: any, editor: any) => {
    console.log(content);
  };

  const setStateToFoo = () => {
    setEditorValue('foo');
  };
  
  return (
    <div>
      <ReactQuill theme={'snow'} onChange={handleChange} value={editorValue} />
      <button onClick={setStateToFoo}>Set State to Foo</button>
    </div>
  );
}

Or are we still supposed to use class components for the component that react-quill is contained within?

It seems like the useState hook gets decoupled from the component after the first re-render that is triggered by changing the state (if you put in a setEditorValue("someString") call on a button) but the handle change continues working (my guess is because the first useState has a closure around an editor value that no longer exists, so after that point the setState is no longer updating the correct backing value).

@alexkrolick
Copy link
Collaborator

@lukevp it should work. Calling the set callback of useState will re-render with a new value for the state; it shouldn't depend on a closure since it's implemented behind the scenes in React based on the call order of the hooks. Are you breaking the rules of hooks by making a call to a top-level hook conditionally?

A runnable example in Codepen or Codesandbox would help.

@lukevp
Copy link

lukevp commented May 6, 2020

@alexkrolick thank you for the quick response!

I have a codepen that reproduces, but it is the first time I am using codepen so apologies if this is not set up correctly. No, I am not breaking at least that rule of hooks (conditional hooks calls), but I may still misunderstand how this should work with my callback function and appreciate any advice you might be able to give.

https://codepen.io/lukevp/pen/JjYprpd
Steps to reproduce:

  • Press button. Text changes from "startValue" to "foo" in both editor and Current Value div.
  • Type text. the text changes in both the editor and the Current Value div
  • Click button, and the setValue no longer changes the editor text.

This is using quill 1.3.5, I am not sure on codepen how to make it reference the beta. But I am seeing the same issue in my app. My app is using:

PS > yarn list --pattern "react-quill"
yarn list v1.21.1
└─ react-quill@2.0.0-beta.2
Done in 0.60s.

@ThePaulMcBride
Copy link
Contributor

I think the problem is that you're using quill in an uncontrolled mode. You need to call setValue with the value returned from onChange.

I've made a small change to your codepen and it seems to work now!

https://codepen.io/ThePaulMcBride/pen/MWaQOXy

@lukevp
Copy link

lukevp commented May 6, 2020

That’s interesting! So it is required for the component to call setValue within the onChange method to keep the component controlled? I misunderstood this part of the doc, since it says that changes to value would be reflected in the editor I thought that I could call setValue independently. Is this an “obvious” thing that I missed, or is it reasonable for this dependency to be unclear? I ask because I am fairly new to React so I don’t know if by calling it “controlled” in a hooks context there is an implication that I also will call the setValue method to prevent the property from getting out of sync? I haven’t used any other components where if I hooked their main “value” property up to a state variable that I had to use one of their callbacks to manually keep them in sync like this.

@alexkrolick
Copy link
Collaborator

In React, "controlled" means the value and onChange props are passed in by the parent; "uncontrolled" means the defaultValue is passed initially and then the parent has to use some other method (like a ref or DOM traversal) to access the current value. Uncontrolled can be better for performance in the case that tracking the state in the parent component (or a global store like Redux) becomes expensive. Controlled is generally a more convenient API because onChange always gives you the latest value, so you can validate it immediately, submit it or transform it in some way as part of your normal state management code.

You'd have the same bug in a regular input field; in fact, React would warn about it. It doesn't for us since it doesn't know about components that aren't standard DOM elements.

https://reactjs.org/docs/uncontrolled-components.html

@lukevp
Copy link

lukevp commented May 7, 2020

Wow. Sorry for the distraction, and thank you both so much for your help and responsiveness. This was a major gap in my understanding of what controlled meant.

@zehawki
Copy link

zehawki commented May 23, 2020

Call out to companies or products using ReactQuill

nextelection.com - we happily use.

@zenoamaro
Copy link
Owner Author

@alexkrolick is there any other big issue left for v2?

@alexkrolick
Copy link
Collaborator

@zenoamaro not that I know of! I think it's good to go.

@hrahimi270
Copy link

There is any way to use it on SSR frameworks without the window condition? I mean this:

const ReactQuill = typeof window === 'object' ? require('react-quill') : () => false;

This way, developers don't have access to props and types.

@alexkrolick
Copy link
Collaborator

@hrahimi270 please open a new issue for providing a built-in SSR workaround

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR introduces a backwards-incompatible change next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet