-
Notifications
You must be signed in to change notification settings - Fork 53
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
Prepare ground for v2 #199
Conversation
BREAKING CHANGE: Move core logic to hook, use TypeScript, use React Testing Library for unit tests, use Nightwatch.js for E2E tests. Use Rollup to build library (umd, esm, cjs).
Refactor e2e tests to async/await synta, adjust BrowserStack Local config (localIdentifier), improve testing scripts (e2e retry, better logging, etc.)
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.
Overally looks good! There are only some minor notes in several places and two bigger general doubts/questions:
-
Shouldn't CJS and ESM builds also be minified?
-
The
umd
sample can't be built. When I'm trying to runnpm install
, I get:npm ERR! code ERESOLVE npm ERR! ERESOLVE unable to resolve dependency tree npm ERR! npm ERR! While resolving: undefined@undefined npm ERR! Found: [email protected] npm ERR! node_modules/react npm ERR! react@"^17.0.2" from the root project npm ERR! npm ERR! Could not resolve dependency: npm ERR! peer react@"^16.0.0" from [email protected] npm ERR! node_modules/ckeditor4-react npm ERR! ckeditor4-react@"latest" from the root project npm ERR! npm ERR! Fix the upstream dependency conflict, or retry npm ERR! this command with --force, or --legacy-peer-deps npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
Probably it's due to the mismatch between sample's deps and deps of the latest published version of
ckeditor4-react
package. It works when I use--legacy-peer-deps
flag. Not sure if we shouldn't add info about it to theCONTRIBUTING.md
file.
Thanks @Comandeer for the review. Regarding your main concerns:
|
Thanks @Comandeer! I have applied the following changes:
Regarding your other comments:
This confirms what I suspected previously: One cannot re-use DOM element for creating new editor instances. Therefore I have memoized
If I understand correctly, this should fix behavior of editor losing iframe context? If so, then in case of React integration this will fix the issue of rendering list of |
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.
- Changed the way how
initData
works. Now it's just a child of root element. In order to hide the content before editor is initialized, I've added a small styling trick.
It seems to work, but please see inline comment about it.
Is it possible to access
beta
version via CDN?
Maybe not beta, but there is nightly.
Looks pretty well, just several small issues. Please also check if code works with the newest nightly (as it includes reattach feature).
Thanks @Comandeer. I've added following changes:
I've given it one more try. I still think that setting initial data via Regarding nightly build. As I mentioned on Slack, I did some testing with both versions of React integration and it looks like no config adjustments are required. Nightly build just works out of the box. And it fixes the issue with re-ordering classic editors 🎉. |
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.
LGTM! 🎉
Could you only rebase it onto major
and merge it?
Within this PR the following has been done:
Single React component has been split into
useCKEditor
hook andCKEditor
component. The first one is supposed to contain core business logic and the latter is a convenience wrapper for simpler use cases. Advanced use cases will be left to userland viauseCKEditor
hook. Please note that business logic is not completed yet. It's just an early scaffold for future improvements (will be continued in Rewrite to hooks #124).TypeScript has been introduced. There is
tsconfig.json
file and TS has been added to build pipeline. Please notice that for the sake of unit testingbabel
is used to transpile TS code. It's due to some performance issues I had with@rollup/plugin-typescript
inside Karma.Rollup is used to create 3 output files, one for each format:
umd
,esm
, &cjs
. Additonally, TS declarations will be emitted.package.json
has been adjusted to contain relevant fields (main
,module
,types
).Samples are contained in
samples
directory as self-contained applications. This approach will allow users to easily fork each sample (via GitHub or other services, e.g. CodeSandbox) for testing and issue reporting.Approach to tests has been changed. Unit tests will run via Karma with the help of React Testing Library. Unit tests will run on all supported versions of React but only on Chrome. The idea here is to just get a feedback if business logic is implemented correctly. In order to test if this library works correctly on all supported browsers, E2E tests are introduced. They are configured to run on all
samples
via Nightwatch.js and BrowserStack. Helperscripts
have been adjusted accordingly. See my comment below for longer explanation.Please read through
CONTRIBUTING.md
before reviewing.CI pipeline might be still unstable though, especially E2E tests...
Closes #193.
Closes #184.
Closes #159.
Closes #124.
A sidenote on testing. I spent some time on choosing right way to test this library. Unit tests should be at the very core of any testing suite. In a React app, this is typically achieved with Jest framework running in
jsdom
environment on Node.js. However, this approach is not compatible withckeditor4
due to editor's custom dependency management system via injectedscripts
. Therefore, unit tests must run on a real browser. This has been achieved with custom Karma config. I believe it is not necessary to run unit tests on all supported browsers in case of this library. Anyway, since we still should test supported browsers somehow, I figured that it might be enough to run some sanity checks onsamples
. Therefore, I've added simple end-2-end tests with help of Nightwatch.js and BrowserStack (there is an official guide). This approach has another benefit: We test library output files and not source code in opposition to unit tests. This way we can ensure that our library can be easily consumed and there are no issues with the build output.