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

Moved utils to ckeditor5-dev-utils package #261

Merged
merged 4 commits into from
Jul 26, 2016
Merged

Moved utils to ckeditor5-dev-utils package #261

merged 4 commits into from
Jul 26, 2016

Conversation

maxbarnas
Copy link
Contributor

@maxbarnas maxbarnas commented Jul 22, 2016

Resolves: #252

WARNING This PR can be closed only after closing those PRs: https://github.com/ckeditor/ckeditor5-dev-utils/pull/2 and https://github.com/ckeditor/ckeditor5-dev-task-lint/pull/2

I updated modules depending on utils, added ckeditor5-dev-utils to
package.json and fixed build task to reflect needed changes in
ckeditor5-dev-utils package.

I updated modules depending on utils, added `ckeditor5-dev-utils` to
`package.json` and fixed build task to reflect needed changes in
`ckeditor5-dev-utils` package.
@maxbarnas maxbarnas changed the title Moved utils to ckeditor5-dev-utils package. Moved utils to ckeditor5-dev-utils package Jul 22, 2016
@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2016

WARNING This PR can be closed only after closing those PRs: ckeditor/ckeditor5-dev-utils#2 and ckeditor/ckeditor5-dev-task-lint#2

Why do we have to close ckeditor/ckeditor5-dev-task-lint#2 before this PR? This PR is only about dev utils, so what has linting to do with it?

@maxbarnas
Copy link
Contributor Author

maxbarnas commented Jul 25, 2016

It is because ckeditor5-dev-utils requires ckeditor5-dev-task-lint.

@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2016

Ah, makes sense :).

@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2016

(t/252 7f9eb46 ?1) p@m /www/ckeditor5/ckeditor5> gulp relink
(node:36332) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
module.js:440
    throw err;
    ^

Error: Cannot find module 'ckeditor5-dev-utils'
    at Function.Module._resolveFilename (module.js:438:15)
    at Function.Module._load (module.js:386:25)
    at Module.require (module.js:466:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/www/ckeditor5/ckeditor5/dev/tasks/build/tasks.js:17:26)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:456:32)
    at tryModuleLoad (module.js:415:12)
    at Function.Module._load (module.js:407:3)
(t/252 7f9eb46 ?1) p@m /www/ckeditor5/ckeditor5> ls node_modules/ | grep ckeditor5-dev-utils
ckeditor5-dev-utils
(t/252 7f9eb46 ?1) p@m /www/ckeditor5/ckeditor5> ls node_modules/ckeditor5-dev-utils/
CHANGES.md  CONTRIBUTING.md LICENSE.md  README.md   dev     gulpfile.js package.json    tests

@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2016

@Reinmar
Copy link
Member

Reinmar commented Jul 25, 2016

PRs in dev-utils and dev-lint are closed, so this PR can now be reviewed. First thing that should be done is running npm-check to remove unnecessary deps.

@@ -35,6 +35,7 @@
"benderjs-promise": "^0.1.0",
"benderjs-sinon": "^0.3.0",
"chai": "^3.4.0",
"ckeditor5-dev-utils": "ckeditor/ckeditor5-dev-utils",
Copy link
Contributor

@szymonkups szymonkups Jul 26, 2016

Choose a reason for hiding this comment

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

Package is located under devDependencies and it feels like a natural way to use such dev-utils package. But this way gulp init task will not put this package in development mode (it only takes dependencies from package.json). Is this intentional? This is utilities package but it is still developed by us, so this might be helpful. Maybe we need some option for gulp init task to also scan devDependencies?

Copy link
Member

Choose a reason for hiding this comment

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

This was intentional. The linting package will be required by all other packages anyway and using links everywhere makes no sense. So I think we should use a normal workflow here with releases and publishing. I even wrote in #215 (comment) that we need to update package.json after we finish refactoring to use versioned packages.

@szymonkups szymonkups merged commit 86cc4f0 into master Jul 26, 2016
@szymonkups szymonkups deleted the t/252 branch July 26, 2016 09:48
mlewand pushed a commit that referenced this pull request May 1, 2020
Fix: Link selection attributes should be cleared after inserting a link via `Model#insertContent()` for better UX. Closes #6053.
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.

Extract development utils to separate package
3 participants