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

Builds should clean the build directory before each npm run build task #700

Closed
ma2ciek opened this issue Nov 30, 2017 · 6 comments
Closed
Assignees
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@ma2ciek
Copy link
Contributor

ma2ciek commented Nov 30, 2017

For now, running npm run build inside one of out three builds just adds webpack output files to the build directory. After merging the #624, it became dangerous, because loading old translation files with the new editor will blow up the UI labels.

@ma2ciek ma2ciek added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. labels Nov 30, 2017
@ma2ciek ma2ciek added this to the iteration 14 milestone Nov 30, 2017
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Nov 30, 2017

I'll add this removal as a plugin to the webpack's config since it's the safest way, because we know exactly the output directory and this is the last step of building the editor.

@ma2ciek ma2ciek self-assigned this Nov 30, 2017
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Nov 30, 2017

Hmm. But assuming, that nobody will call the webpack from these packages, we can add rm -rf build to the build-ckeditor.sh, so it will be called during the build-ckeditor task.

@szymonkups
Copy link
Contributor

szymonkups commented Nov 30, 2017

I would use webpack plugin to do this and leave build-ckeditor.sh as an entry point. I think it will be a OS-safe solution.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Nov 30, 2017

And like you said, it'll give a developer some insights, that this part is needed for some reason.

@Reinmar
Copy link
Member

Reinmar commented Dec 1, 2017

The problem here is not critical. Nothing bad will happen if some old language files will be kept. So it's not a problem if people would forget about this somehow.

The second point is that only the lang directory is a problem, am I right? This directory is managed by the CKEditorWebpackPlugin(). If it's not loaded, then it won't be created at all. So, cleaning up this directory should be the role of this plugin. So wherever and whenever you'll use it, the cleanup will be done.

Installing a separate webpack plugin to solve accountability of another piece of code is not a good solution. It reminds me grunt times when everything was maintained in a configuration meaning that configuration was crucial to do things right. And configuration is already hard to maintain. Every change like the one proposed in ckeditor/ckeditor5-build-balloon#5 needs to be then copied to every place where we document our plugin's usage. And everyone who already had it configured would need to know that something has changed.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Dec 1, 2017

I have to agree with you. I'll try to do it nicely on the CKEditorWebpackPlugin's side.

szymonkups added a commit to ckeditor/ckeditor5-dev that referenced this issue Dec 5, 2017
Fix: Added language directory cleaning before each webpack build. Closes ckeditor/ckeditor5#700.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants