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

[Umbrella] Proposal for next major version #189

Closed
MMMalik opened this issue Apr 7, 2021 · 2 comments
Closed

[Umbrella] Proposal for next major version #189

MMMalik opened this issue Apr 7, 2021 · 2 comments
Labels
status:confirmed An issue confirmed by the development team.
Milestone

Comments

@MMMalik
Copy link
Contributor

MMMalik commented Apr 7, 2021

Are you reporting a feature request or a bug?

Feature

Provide details

TL;DR

Migration to hooks is advisable. Since it's going to introduce next major version of this library, let's focus on changing other aspects too: official support for React v17, introdution of TypeScript and replacement of Enzyme with React Testing Library.

Motivation

I believe we can start a discussion about next major version. As pointed out by @Comandeer in #124 we could benefit from re-writing component-based approach to a hook-based one. Since this would be a breaking change, we can also take the opportunity to focus on other major feature requests (React v17, TypeScript, support for "webpackless" build). Below I have summarized topics that I believe can be tackled as part of next major version.

Rewrite to hooks #124

Hooks are the modern way of abstracting business logic in a React app. Although there is no danger that classes are going to be removed from React, moving to hooks / functional components will help us get in line with modern React design patterns. For greater flexibility on the library consumer end, I suggest that we primarily expose useCKEditor hook. Its responsibility would be to initialize an instance of the editor and expose it. Of course CKEditor functional component (with similar interface as the current one) can be exposed too in order to facilitate v1 -> v2 migration (it might be also useful for simpler use cases).

Possible usage (simplified):

const MyEditor = ({ editorUrl, type }) => {
  const ref = useRef();
  const { editor } = useCKEditor({ editorUrl, type, element: ref });

  useEffect(() => {
    if (editor.status === 'instanceReady') {
      getDataFromApi().then((data) => editor.setData(data));
    }
  }, [editor.status]);  

  return <div ref={ref} />
}

Proposed change: Expose useCKEditor hook as default

Pros

  • In line with modern React design patterns
  • Allows greater flexibility for consumers

Cons

  • Well, a rewrite is necessary

Support React v17 #159

Once rewrite to hooks is completed, React v16.8 becomes our minimum supported version. Since React v17 has not introduced any breaking changes yet (in relation to v16), we should be fine with supporting React v17 also. There is a different problem though. As outlined in #159, Enzyme lacks support for React v17. In addition, the support for hooks in Enzyme is still unsatisfactory. One solution to the shortcomings of Enzyme would be to switch to a different testing framework. There is recently a huge increase in popularity of React Testing Library (RTL) which overtook Enzyme some time ago. Also, RTL encourages different approach to testing than Enzyme (testing from end-user perspective vs testing implementation details).

Proposed change: Drop Enzyme and use React Testing Library instead

Pros:

  • RTL has great support for modern React
  • RTL is more popular than Enzyme, especially for newer React projects
  • RTL encourages different, more popular approach to testing React components / hooks

Cons:

  • Overhead of learning new framework
  • We lose possibility to re-use existing testing logic

Support TypeScript / TS definitions #82

We can either continue to use JavaScript and add TS definitions in a separate file or we can use TypeScript from the ground up. The overhead associated with using TS in the whole project is not that significant. It basically boils down to typing interfaces (props in case of a component and arguments in case of a hook).

Proposed change: Use TypeScript from the ground up

Pros:

  • Built-in support for emitting types declarations
  • Strong type checking

Cons:

  • It's a new language (even if it's just superset of JS)

Support build w/o webpack's overhead #180

Currently, we only emit umd build. We should expose cjs and es modules too for greater flexibility. Currently we're using Webpack, but there other possibilities: Rollup, TypeScript CLI, Babel CLI?

Proposed change: ?

@MMMalik MMMalik added the status:confirmed An issue confirmed by the development team. label Apr 8, 2021
@f1ames f1ames changed the title Proposal for next major version [Umbrella] Proposal for next major version Apr 8, 2021
@MMMalik MMMalik added this to the 2.0.0 milestone Apr 9, 2021
@f1ames
Copy link
Contributor

f1ames commented Apr 9, 2021

We also have #123 which can give some additional information how we would like to approach samples in the new major version.

@f1ames
Copy link
Contributor

f1ames commented Jul 26, 2021

Covered with 2.0.0 version (f03dde8 and https://www.npmjs.com/package/ckeditor4-react/v/2.0.0).

@f1ames f1ames closed this as completed Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed An issue confirmed by the development team.
Projects
None yet
Development

No branches or pull requests

2 participants