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

Fixes #19478 - migrate to webpack2 #4514

Merged
merged 1 commit into from Jun 26, 2017

Conversation

matanwerbner
Copy link
Contributor

As webpack 1 is being deprecated, we better migrate to 2 to have full support and updates.

@matanwerbner
Copy link
Contributor Author

storybook works fine as well.

package.json Outdated
@@ -56,6 +56,7 @@
"jstz": "~1.0.7",
"lodash": "~4.15.0",
"multiselect": "~0.9.12",
"fbjs": "0.8.12",
Copy link
Member

Choose a reason for hiding this comment

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

is it required by webpack2 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ohadlevy
Copy link
Member

ohadlevy commented May 9, 2017

[test foreman]

@ohadlevy
Copy link
Member

ohadlevy commented May 9, 2017

[test katello]

package.json Outdated
@@ -36,7 +35,7 @@
"stats-webpack-plugin": "^0.2.1",
"style-loader": "^0.13.1",
"url-loader": "^0.5.7",
"webpack": "^1.9.11",
"webpack": "^2.5.1",
Copy link
Member

Choose a reason for hiding this comment

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

when testing it i got:

error extract-text-webpack-plugin@2.1.0: The engine "node" is incompatible with this module. Expected version ">=4.3.0 < 5.0.0 || >= 5.10".
error Found incompatible module

any idea what are the new minimum requirements? I assume this is going to effect our build infra too.
/cc @tbrisker

Copy link
Member

Choose a reason for hiding this comment

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

this is probably a local issue that i had node v5.0.0 by mistake.

Copy link
Member

Choose a reason for hiding this comment

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

@matanwerbner 2.6.1 is already the latest, can you revisit all the dependencies mentioned here?

@ohadlevy
Copy link
Member

ohadlevy commented May 9, 2017

@matanwerbner I've noticed its slower, do you think wepack v2 is ready for "production"?

@matanwerbner
Copy link
Contributor Author

@ohadlevy it is definatley pro

@matanwerbner matanwerbner reopened this May 9, 2017
@matanwerbner
Copy link
Contributor Author

Definatley prod ready although i seen many user experiencing slower build times, and reverting to v1 (which is officialy deprecated).
We can probably hold off on this for a while.

package.json Outdated
@@ -36,7 +35,7 @@
"stats-webpack-plugin": "^0.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

need to bump version

@ohadlevy
Copy link
Member

also need to change the - webpack.optimize.OccurenceOrderPlugin(), to webpack.optimize.OccurrenceOrderPlugin
but it might not be required at all anymore - see webpack/webpack#1964 and https://gist.github.com/sokra/27b24881210b56bbaff7#occurrence-order

@ohadlevy
Copy link
Member

ohadlevy commented Jun 1, 2017

@matanwerbner I've been using it over a week on my instance without a significant impact, I'm fine with merging this.

@matanwerbner
Copy link
Contributor Author

@ohadlevy i have rebased, feel free to merge :)

@matanwerbner
Copy link
Contributor Author

@ohadlevy tests are good

@ohadlevy
Copy link
Member

@matanwerbner seems to work, I am however seeing the following:

WARNING in webpack: Using NoErrorsPlugin is deprecated.
Use NoEmitOnErrorsPlugin instead.

@ohadlevy
Copy link
Member

setting as WoC as I'm still getting

WARNING in webpack: Using NoErrorsPlugin is deprecated.
Use NoEmitOnErrorsPlugin instead.

@ohadlevy
Copy link
Member

@matanwerbner
Copy link
Contributor Author

guess so... 😆

@matanwerbner
Copy link
Contributor Author

@ohadlevy it's v3 now, no improvement in the file size, however bundle is structured a bit differently, in a way that is supposed to improve performance.
this is freshly rebased.

@ohadlevy
Copy link
Member

nice! can you just address:

Chunk.modules is deprecated. Use Chunk.getNumberOfModules/mapModules/forEachModule/containsModule instead.

@matanwerbner
Copy link
Contributor Author

we need to watch this space:
webpack-contrib/extract-text-webpack-plugin#543

@ohadlevy
Copy link
Member

I also see

warning "extract-text-webpack-plugin@2.1.2" has incorrect peer dependency "webpack@^2.2.0".

should we bump extract-text version?

@matanwerbner
Copy link
Contributor Author

matanwerbner commented Jun 20, 2017

@ohadlevy we are at latest, nowhere to bump.
extract-text-plugin does not yet support webpack v3...hence the PR i've attached.

@ohadlevy
Copy link
Member

also..

warning "extract-text-webpack-plugin@2.1.0" has incorrect peer dependency "webpack@^2.2.0".
warning "sass-loader@4.1.1" has incorrect peer dependency "webpack@^2 || ^2.2.0-rc.0 || ^2.1.0-beta || ^1.12.6".
warning "stats-webpack-plugin@0.6.0" has incorrect peer dependency "webpack@^1.0||^2.1.0-beta||^2.2.0-rc".
warning "webpack-dev-server@1.16.5" has incorrect peer dependency "webpack@>=1.3.0 <3".

@matanwerbner
Copy link
Contributor Author

@ohadlevy should we go back to v2, and wait for 3 to stabilize?

@ohadlevy
Copy link
Member

ohadlevy commented Jun 22, 2017 via email

@matanwerbner
Copy link
Contributor Author

Hard to say, it seems like no braking changes have occured, i however would feel more comfortable to wait with it.
also, it doesn't bring any significant improvements to the table... your call :)

@ohadlevy ohadlevy merged commit 114ea49 into theforeman:develop Jun 26, 2017
@ohadlevy
Copy link
Member

thanks @matanwerbner

@ohadlevy
Copy link
Member

packaging pr at theforeman/foreman-packaging#1693

ekohl added a commit to ekohl/foreman that referenced this pull request Nov 27, 2017
This was needed as a workaround[1] that's no longer needed.

[1]: theforeman#4514 (comment)
tbrisker pushed a commit that referenced this pull request Nov 27, 2017
This was needed as a workaround[1] that's no longer needed.

[1]: #4514 (comment)
mmack pushed a commit to mmack/foreman that referenced this pull request Dec 13, 2017
This was needed as a workaround[1] that's no longer needed.

[1]: theforeman#4514 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants