-
Notifications
You must be signed in to change notification settings - Fork 936
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
Conversation
Oh dang 😃. I'll look at this later |
that's a great chunk of work 👍 |
Oh man, this looks good! |
👏 |
Typescript is a good language! Hopes its work could be push. Thanks :) |
Looking forward to this beta release! If it goes beta, will there be a publish to the unpkg CDN too? |
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? |
@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. |
* 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 [email protected] * 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
* 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 [email protected] * 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
* 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 [email protected] * 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
@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? 🙂 |
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. |
4b34ab0
to
5fbdece
Compare
@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 I want to achieve a few things for this beta:
After the beta:
|
I have fixed the 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 |
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might actually work now, worth taking a look
7e5f69b
to
a442c09
Compare
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). |
@lukevp it should work. Calling the set callback of A runnable example in Codepen or Codesandbox would help. |
@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
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. |
I think the problem is that you're using quill in an uncontrolled mode. You need to call I've made a small change to your codepen and it seems to work now! |
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. |
In React, "controlled" means the You'd have the same bug in a regular |
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. |
nextelection.com - we happily use. |
@alexkrolick is there any other big issue left for v2? |
@zenoamaro not that I know of! I think it's good to go. |
There is any way to use it on SSR frameworks without the const ReactQuill = typeof window === 'object' ? require('react-quill') : () => false; This way, developers don't have access to props and types. |
@hrahimi270 please open a new issue for providing a built-in SSR workaround |
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
anddist
will be included in the package, so users should be fine. I will additionally switchrm -rf
torimraf
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.
2.0.0-beta1
to NPMmaster
v2.0.0