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

Use CKEditor5 as dependency #145

Closed
3 tasks done
jodator opened this issue Apr 28, 2016 · 5 comments
Closed
3 tasks done

Use CKEditor5 as dependency #145

jodator opened this issue Apr 28, 2016 · 5 comments
Assignees
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@jodator
Copy link
Contributor

jodator commented Apr 28, 2016

As the project will be used by others it would be nice to have:

  • installing ckeditor5 with currently "dev" dependencies as regular dependencies for gulp buil
  • making gulp build aware of directory structure (as for now it assumes that all ckeditor5-* repositories are in node_modules folder of ckeditor5 repository)
  • making some gulp tasks optional (ie. currently some tasks reads .gitignore file that is not present when ckeditor is installed as some project's dependency via npm install)
@jodator jodator self-assigned this Apr 28, 2016
@jodator jodator added type:question This issue asks a question (how to...). and removed type:question This issue asks a question (how to...). labels Apr 28, 2016
@Reinmar Reinmar added module:builder type:improvement This issue reports a possible enhancement of an existing feature. type:feature This issue reports a feature request (an idea for a new functionality or a missing option). labels Apr 29, 2016
@pjasiun pjasiun changed the title Use CKEditor5 as dependency Maciek must be able to run CKEditor Apr 29, 2016
@pjasiun pjasiun changed the title Maciek must be able to run CKEditor Use CKEditor5 as dependency Apr 29, 2016
@Reinmar
Copy link
Member

Reinmar commented May 5, 2016

  • We should add --build-dir param to the command, but the default should be set as you coded it now,
  • Not loading some of the gulp tasks when in dev mode is generally a good idea, however, we could go step further (if you wish to spend time on this) and move require() calls to the places where they are used. I've read on gulp's issue tracker that the performance issues that we have with it (bootstrap time) are caused by the fact that we always require dozens of modules, even though a currently run task needs a small subset of them. OTH, this is kind of a separate issue.
  • let isDev = !json._id; – this should be a const.
  • I think we don't use almond anywhere now, so you can remove it from package.json.
  • Just a note for clarity – the package directories finding logic will need to be much more complex than what we have now, because the deeps tree will at some point become deeper. ckeditor5's dependencies which are installed in a dev mode (as a linked repo) will have their own dependencies inside themselves. This is absolutely not covered now, so we build from a flat tree.

That would be it I guess. Don't forget about tests (npm test) and this will be ready for a merge.

@jodator
Copy link
Contributor Author

jodator commented May 5, 2016

Thanks for the insights. I'll get them when beautifying PR.

@jodator
Copy link
Contributor Author

jodator commented May 5, 2016

Finally checked. Both builds are equal for gulp build:js --formats=cjs.

@jodator
Copy link
Contributor Author

jodator commented May 6, 2016

@Reinmar I've just fund that guppy or dev/tasks are too aggressive. They installed hooks in top-level repo (probably before I've made changes in t/145 branch). I'll check that also.

@Reinmar
Copy link
Member

Reinmar commented Aug 16, 2016

Can't this ticket be closed now after #262?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

2 participants