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

Vue 3 Support #158

Closed
nfplee opened this issue Sep 29, 2020 · 11 comments · Fixed by #165
Closed

Vue 3 Support #158

nfplee opened this issue Sep 29, 2020 · 11 comments · Fixed by #165
Assignees
Labels
squad:integrations type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@nfplee
Copy link

nfplee commented Sep 29, 2020

Now Vue 3 has been released, do you know when you'll support it?

@FilipTokarski FilipTokarski added the resolution:duplicate This issue is a duplicate of another issue and was merged into it. label Sep 29, 2020
@FilipTokarski
Copy link
Member

Hi thanks for the report. We are aware of this and we're tracking this in the main CKEditor5 repo. If you would like to see it implemented faster, please add 👍 there, to bump it's popularity. Of course also feel free to add there anything you consider important.
I'll close this one as a duplicate.

@oleq
Copy link
Member

oleq commented Oct 8, 2020

As per my ckeditor/ckeditor5#8135 (comment) it looks like the implementation requires changes to become Vue 3.x-compatible. I'm re-opening this issue to keep track of the development and I leave the issue in ckeditor5 open for documentation only.

@oleq oleq reopened this Oct 8, 2020
@oleq oleq added type:task This issue reports a chore (non-production change) and other types of "todos". squad:integrations and removed resolution:duplicate This issue is a duplicate of another issue and was merged into it. labels Oct 8, 2020
@oleq oleq added this to the iteration 37 milestone Oct 8, 2020
@oleq
Copy link
Member

oleq commented Oct 9, 2020

Quoting myself from ckeditor/ckeditor5#8135 (comment) because an idea just popped into my head:

I confirm the comment by @nfplee. The render() function syntax has changed and to work in Vue 3.x, the CKEditor component must use the h() function imported like this import { h } from 'vue' (see the current imlementation).

  • This means we need to do this change in the pre-built component we distribute via npm.
  • However, unless we update Vue to 3.x in @ckeditor/ckeditor5-vue, a warning is thrown when building the component "export 'h' was not found in 'vue' and the component does not work in a Vue 3.x application.
  • Updating Vue to 3.x in @ckeditor/ckeditor5-vue and using import { h } from 'vue' obviously breaks compatibility with Vue 2.x.

What if we compiled (distributed) the component using Vue 3.x in ckeditor5-vue? If h() imported in Vue 3.x was truly the same as the rendering callback provided by render( callback ) {} in Vue 2.x, this could work in both versions. 

Then we'd only need to resolve beforeDestroy and beforeUnmount story, which sounds easy–peasy. 

cc @psmyrek

@psmyrek
Copy link
Contributor

psmyrek commented Oct 14, 2020

TL;DR

Current status:

  • CKEditor component source code is common for both Vue versions.
  • All tests for Vue 2.x are passing.

To do:

  • Try to adapt existing tests for Vue 3.x.
  • In order to have one codebase for both Vue versions, both Vue versions and both Vue Test Utils versions (and maybe also something else?) have to be available (installed?) for unit tests. Question is how to do that without getting them mixed up together and be able to switch between them easily before running tests?
    • The one and only proposal I have right now is to install all dependencies that need to be switched in aliased names, like npm i vue-v2@npm:vue vue-v3@npm:vue@next (and the same for other dependencies) and then just before running tests the npm link node_modules/<aliased name> could be used to "install" desired version. The npm link uses real module name from its package.json file so npm link node_modules/vue-v3 "installs" Vue 3.x in node_modules/vue folder, as expected. I heard that @ma2ciek could help me here - maybe there are simpler ways to do that and I'm missing something? 🙂
  • Update documentation.

Detailed notes what have been already investigated and done

After a several hours of digging in the migration guide and doing a lot of experiments, I was able to adapt the CKEditor component for both versions of Vue. The main change that I've made is that src/ckeditor.js and src/plugin.js must return a function that will determine the shape of the returned objects based on the Vue version. This change is necessary because there are 2 fundamental differences between both Vue versions (mentioned already in previous comments) that cannot be shared:

  • The beforeDestroy lifecycle hook from Vue 2.x has been renamed to beforeUnmount in Vue 3.x and having the former one in Vue 3.x displays a warning in console (which is not nice).
  • The component's render function in Vue 2.x automatically receives the h function (which is a conventional alias for createElement function) as the first argument. In 3.x, the h has to be globally imported from Vue instead of being automatically passed as an argument to render.

Other changes that I was able to unify in CKEditor component for both versions:

  • The install method in the plugin object returned from src/plugin.js in Vue 2.x has Vue constructor as the first argument, but in Vue 3.x the first argument in install method is the application instance from Vue's createApp method. Luckily, plugins are installed in the same way in both Vue versions: by calling component method on the first argument passed to install method (on Vue constructor for Vue 2.x or on application instance for Vue 3.x), so nothing needs to be changed here.
  • The v-model directive used on custom component has been changed in Vue 3.x, where it is now an equivalent of passing a modelValue prop and emitting an update:modelValue event. Fortunately components can define own prop and event names for v-model so these changed names were ported back for Vue 2.x.
  • There was also one additional breaking change related to v-model that surprised me a bit and it is not even mentioned in Vue migration guide: in Vue 3.x it is not possible to have more than one listener for a particular event, as it is reported here. Due to that fact CKEditor component must emit two different events: first one - update:modelValue - only for the v-model and second one - input - for someone who wants to explicitly listen to editor data changes and to not break the existing CKEditor component API.

There is one thing that is not possible to implement in Vue 3.x: programmatically unmounting (or destroying) Vue component from the DOM element. In Vue 2.x component instance have the this.$destroy method which completely destroy a component instance, cleans up its connections with other existing instances, unbind all its directives and turn off all event listeners. In Vue 3.x the $destroy method has been removed and users should no longer manually manage the lifecycle of individual Vue components.. There is an unmount method on the application instance, but it works totally different than $destroy from Vue 2.x: it just removes all nodes from the mounting point in the DOM.

And of course the component initialization/installation is totally different between these versions but this is nicely described in official guide.

CC: @oleq, @mlewand, @Reinmar

@oleq
Copy link
Member

oleq commented Oct 15, 2020

Thanks, @psmyrek! That's awesome work you did there.

I skimmed through the PR and it got me thinking, is there any way we can run all these tests twice when npm run test? Like once for Vue 2.x and the second time for Vue 3.x? Because it looks like it runs tests against one version of Vue only in your PR.

There's this entry point for tests that could do const config = getKarmaConfig(); twice with different arguments (Vue versions):

const config2 = getKarmaConfig( 'vue-2' );
const config3 = getKarmaConfig( 'vue-3' );

new KarmaServer( config2 ).start();
new KarmaServer( config3 ).start();

getKarmaConfig() has the webpackConfig section where we could use some dependency injection technique (like ProvidePlugin, maybe there's a better one @pomek?) so when tests run for the first time import Vue imports Vue 2.x, and the second time they run it is Vue 3.x.

@oleq
Copy link
Member

oleq commented Oct 15, 2020

maybe there's a better one @pomek

Is it as simple as https://webpack.js.org/configuration/resolve/#resolvealias?

@pomek
Copy link
Member

pomek commented Oct 15, 2020

We could create two aliases, for Vue 2 and 3. And import proper packages by using those aliases.

import Vue2 from 'Vue@2'
import Vue3 from 'Vue@3'

It should work when you define aliases in config:

module.exports = {
  //...
  resolve: {
    alias: {
      'Vue@2': /* ... */,
      'Vue@3': /* ... */
    }
  }
};

@psmyrek
Copy link
Contributor

psmyrek commented Oct 16, 2020

Thank you for hints and ideas - they were all very helpful! 👍🏻

I've just updated my PR #160 and now all existing tests work for both Vue versions and we can run them for both Vue versions using npm run test - as requested by @oleq. Of course it is also possible to run all tests only for specific Vue version: npm run test:v2 targeting Vue 2.x or npm run test:v3 for Vue 3.x.

Regarding the problem with dependencies (the Vue framework itself and the Vue Test Utils for tests) and their versions, I realized that I can't use my previous idea (which then was also improved by @pomek) to just alias Vue 2.x and Vue 3.x versions in webpack configuration as Vue@2 and Vue@3. The reason for this is that installing these packages adds also other related (required) packages, so aliasing just Vue framework might be not enough - otherwise we wouldn't be 100% sure that we did not mix up anything. For example Vue 3.x adds 15 packages to node_modules and Vue Test Utils for Vue 2.x installs 35 packages. Moreover, using aliases in webpack is also not sufficient because Vue Test Utils internally imports Vue framework using the vue name, and not Vue@2 or Vue@3. To solve this problem I've used webpack's resolve.modules configuration option (instead of resolve.alias) to explicitly define a directory to search for a package (and its all dependencies) and which takes precedence over default node_modules from the root. Then I've created two package.json files in dependencies/vue-v2 and dependencies/vue-v3 folders to specify Vue-related packages for version 2.x and 3.x, respectively. This approach guarantees that we will use exactly the same "Vue & Friends" packages in tests like in normal, single-Vue-version projects. To make this whole solution more developer friendly, installing dependencies from root folder using npm install automatically installs also all necessary Vue-related packages in dependencies/vue-v<version>/node_modules, so calling only npm i && npm test from the root folder starts tests for both Vue versions.

After splitting up the test into two separate test runs (one for each Vue version), the code coverage reports are also separated. In my opinion we should merge them because the component code targets both Vue versions, so in the same way the overall code coverage report should also reflect that fact.

Things to do next:

  • Clean up the code (remove duplicated code, add/update documentation in code, get rid of warnings from console).
  • Merge code coverage reports from two separate test runs into one.
  • Update documentation.

@Reinmar Reinmar modified the milestones: iteration 37, iteration 38 Oct 26, 2020
@nfplee
Copy link
Author

nfplee commented Oct 30, 2020

@psmyrek any update on this? I saw it missed iteration 37. Are there plans to include this in the next release?

@psmyrek
Copy link
Contributor

psmyrek commented Nov 2, 2020

This issue is more complicated than it appeared at first. The related PR #160 contains working component code for two versions of Vue, but it contains also various hacks which we want to get rid of. For this reason, we need more time to prepare a better solution.

@psmyrek psmyrek mentioned this issue Nov 3, 2020
@nfplee
Copy link
Author

nfplee commented Nov 3, 2020

@psmyrek thanks for the update.

pomek added a commit that referenced this issue Nov 20, 2020
Feature: `CKEditor` component supports Vue 3.x. Closes #158.

BREAKING CHANGE: The `CKEditor` component no longer supports Vue 2.x.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:integrations type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants