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

Prepare ground for v2 #199

Merged
merged 55 commits into from
Jun 4, 2021
Merged

Prepare ground for v2 #199

merged 55 commits into from
Jun 4, 2021

Conversation

MMMalik
Copy link
Contributor

@MMMalik MMMalik commented Apr 21, 2021

Within this PR the following has been done:

  1. Single React component has been split into useCKEditor hook and CKEditor 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 via useCKEditor 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).

  2. 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 testing babel is used to transpile TS code. It's due to some performance issues I had with @rollup/plugin-typescript inside Karma.

  3. 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).

  4. 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.

  5. 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. Helper scripts 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 with ckeditor4 due to editor's custom dependency management system via injected scripts. 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 on samples. 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.

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).
@MMMalik MMMalik marked this pull request as draft April 21, 2021 14:18
@Comandeer Comandeer self-requested a review April 23, 2021 08:08
@Comandeer Comandeer assigned Comandeer and unassigned Comandeer Apr 23, 2021
Refactor e2e tests to async/await synta, adjust BrowserStack Local config (localIdentifier), improve testing scripts (e2e retry, better logging, etc.)
@Comandeer Comandeer self-assigned this Apr 26, 2021
Copy link
Member

@Comandeer Comandeer left a 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 run npm 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 the CONTRIBUTING.md file.

.github/workflows/check-pr.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
samples/basic/src/App.jsx Outdated Show resolved Hide resolved
samples/umd/build.js Outdated Show resolved Hide resolved
samples/umd/package.json Outdated Show resolved Hide resolved
@MMMalik
Copy link
Contributor Author

MMMalik commented Apr 27, 2021

Thanks @Comandeer for the review.

Regarding your main concerns:

  1. I don't see many use cases for minified versions of esm and cjs packages at this moment. Most likely a React app developer would use our library with the help of a bundler anyway, so I believe it's better to perform such transformations downstream.
    If at any point in the future community needs minified versions of these packages, then we can consider adding them (it won't be a breaking change).

  2. Regarding failing build. Please note that npm link ckeditor4-react must be run in every sample separately and after every npm install in that sample (install will get rid of links). Try again please - it should be working fine. I've added some clarification to CONTRIBUTING.

@MMMalik MMMalik requested a review from Comandeer April 27, 2021 09:55
@MMMalik
Copy link
Contributor Author

MMMalik commented May 24, 2021

Thanks @Comandeer!

I have applied the following changes:

  1. Changed ssr default port.
  2. 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.
  3. Memoized config and type props. See explanation below.
  4. Added a possibility to listen to custom events. This is done via generic interface (both in CKEditor component and useCKEditor hook). In order to achieve that I had to refactor types related to event handling. There is now a new verbose CKEditorEventHandlerProp interface instead of dynamic template literals. This has one advantageous side-effect though: Now we support TypeScript as low as version 3.5. I've added new test cases and adjusted event samples.

Regarding your other comments:

SCAYT button is active, but clicking does nothing.
Probably it can be connected with the fact that the new editor is bound to the same element as the previous one.

This confirms what I suspected previously: One cannot re-use DOM element for creating new editor instances. Therefore I have memoized config and type props. Now re-mounting of CKEditor component must be done in userland via keyed component (pass unique key prop). This behavior is now quite similar to what we have in v1. I have refactored samples accordingly.

I'm also wondering if the current code is able to handle changes proposed in ckeditor/ckeditor4#4601 (so reattaching the editor to the DOM)?

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 CKEditor components. I believe it should not affect React integration as long as editor handles that under the hood. It would be great though if I could test this new feature before its release. Is it possible to access beta version via CDN?

@MMMalik MMMalik requested a review from Comandeer May 24, 2021 12:20
@Comandeer Comandeer self-assigned this May 26, 2021
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

  1. 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).

src/CKEditor.tsx Show resolved Hide resolved
src/CKEditor.tsx Show resolved Hide resolved
src/CKEditor.tsx Outdated Show resolved Hide resolved
@MMMalik
Copy link
Contributor Author

MMMalik commented May 31, 2021

Thanks @Comandeer.

I've added following changes:

  • initialized config in CKEditor component
  • bumped editor version to 4.16.1
  • fixed root styling for initData

It seems to work, but please see inline comment about it.

I've given it one more try. I still think that setting initial data via children is slightly more elegant solution than using editor.setData imperatively. I will use the latter way as a last resort though.

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 🎉.

@MMMalik MMMalik requested a review from Comandeer May 31, 2021 16:57
@Comandeer Comandeer self-assigned this Jun 1, 2021
Copy link
Member

@Comandeer Comandeer left a 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?

@MMMalik MMMalik merged commit 8caeaf0 into major Jun 4, 2021
@MMMalik MMMalik deleted the t/193 branch June 4, 2021 14:33
@f1ames f1ames mentioned this pull request Jun 4, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants