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.x support #160

Closed
wants to merge 1 commit into from
Closed

Vue 3.x support #160

wants to merge 1 commit into from

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Oct 14, 2020

Suggested merge commit message (convention)

Feature: Added support for Vue 3.x. Closes #158.

Internal: Merged code coverage reports from test runs from both Vue versions before sending them to Coveralls. Closes #162.

BREAKING CHANGE: Now, the CKEditor is a function that accepts an object as its one and only argument with the version property. For Vue 3.x also the h function is required during initialization and it must be passed to this object argument.

BREAKING CHANGE: The prop name used by v-model directive has changed from value to model-value.

@coveralls
Copy link

coveralls commented Oct 14, 2020

Pull Request Test Coverage Report for Build 179

  • 37 of 37 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 143: 0.0%
Covered Lines: 38
Relevant Lines: 38

💛 - Coveralls

@psmyrek psmyrek mentioned this pull request Oct 16, 2020
@psmyrek
Copy link
Contributor Author

psmyrek commented Oct 19, 2020

There are some strange errors in CI build:

$ npm install
npm WARN checkPermissions Missing write access to /home/travis/build/ckeditor/ckeditor5-vue/node_modules/@vue/test-utils
npm WARN checkPermissions Missing write access to /home/travis/build/ckeditor/ckeditor5-vue/node_modules/vue
npm ERR! code ENOENT
npm ERR! syscall access
npm ERR! path /home/travis/build/ckeditor/ckeditor5-vue/node_modules/@vue/test-utils
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, access '/home/travis/build/ckeditor/ckeditor5-vue/node_modules/@vue/test-utils'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent 

@pomek @oleq Have you ever seen them before?
Re-triggering build does not help 😞

@psmyrek psmyrek changed the title [WIP] Vue 3.x support Vue 3.x support Oct 20, 2020
@psmyrek
Copy link
Contributor Author

psmyrek commented Oct 21, 2020

The following things will have to be done before merging this PR:

  • CI build is failing:
npm WARN checkPermissions Missing write access to /home/travis/build/ckeditor/ckeditor5-vue/node_modules/@vue/test-utils
npm WARN checkPermissions Missing write access to /home/travis/build/ckeditor/ckeditor5-vue/node_modules/vue

@oleq oleq self-requested a review October 26, 2020 11:57
package.json Outdated Show resolved Hide resolved
src/plugin.js Outdated Show resolved Hide resolved
scripts/test.js Outdated Show resolved Hide resolved
scripts/test.js Outdated Show resolved Hide resolved
scripts/utils/webpackconfig-vue-v2.js Outdated Show resolved Hide resolved
scripts/utils/webpackconfig-vue-v3.js Outdated Show resolved Hide resolved
src/ckeditor.js Outdated Show resolved Hide resolved
tests/_utils/vueadapter.js Outdated Show resolved Hide resolved
src/helper.js Outdated Show resolved Hide resolved
@psmyrek psmyrek requested a review from oleq October 28, 2020 12:15
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Everything looks good to me except for the "Executing tests" section of the README.md file. The configuration options listed there no longer make sense and some new scripts (like npm run coverage) were included so I think it's worth updating.

If bringing support for test watcher or reporter options is a big issue now, let's drop the support for these. We can always get back and implement them and they're not critical for a small project like this one.

@psmyrek
Copy link
Contributor Author

psmyrek commented Oct 30, 2020

README.md has been updated.

@psmyrek
Copy link
Contributor Author

psmyrek commented Nov 3, 2020

Closed as the proposal to re-use one codebase for both Vue versions is not ideal and requires some hacks to work properly.
New PR has been created: #165

@psmyrek psmyrek closed this Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge code coverage reports from test runs from both Vue versions Vue 3 Support
3 participants