-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Webpack3 (no es6 changes) #4800
Conversation
Is this still considered optional given that Webpack 1x's life support was switched off in April? |
It says optional as I am not member of the riot Team :) I am just not in the Position to decide |
/me wonders why no one wants to review this :P |
I think this might solve real issues: https://matrix.to/#/!DdJkzRliezrwpNebLk:matrix.org/%2415137819892616558FrkqI:matrix.org Any chance of getting this merged? |
Friendly bump😇 |
It might make sense to use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the context on this is that we'd been reluctant to introduce changes to the platform while we were trying to get things stable. Now 0.14 is, it's a better time to be looking at making changes like this.
I haven't actually tried to run this yet, but have commented on the two things I don't understand.
@@ -32,4 +32,4 @@ addons: | |||
install: | |||
# clone the deps with depth 1: we know we will only ever need that one | |||
# commit. | |||
- scripts/fetch-develop.deps.sh --depth 1 && npm install | |||
- npm install && scripts/fetch-develop.deps.sh --depth 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely remember asking you about this back when you submitted the PR... if I did, I can't remember the conclusion though. Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was the CI being stupid. It works this way not the other way. But I have to admit I do not know if this still is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was propably the symlink issue that npm has which later got finished
htmlWebpackPlugin.files.js.splice(i, 1); | ||
} | ||
} | ||
for (var i=0; i < htmlWebpackPlugin.files.js.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You're putting olm first? Why?
- The
array
variable is unused. - Indents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Because in webpack 3 they get randomly ordered which causes olm to sometimes not load. The old script relied on the case that the array is always ordered the same way.
2 and 3 can I fix after the weekend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbkr I am not sure about 3. Should I do 2 spaces or 4? the html uses 2 spaces. Or do you mean the if that is missing one space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. Yeah, I can see how that would break. Generally all the indents should be 4 spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I will leave the html as is but fix the Js code in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I wonder if we should be using chunksSortMode
for the ordering (although given the docs don't tell you anything about what the different modes do, perhaps your solution is better...)
Just tried this and it looks like it's failing to run the postcss preprocessor?
|
Hmm maybe some things got too old. I will after the weekend (I am on a Event and not at a PC) test this again and Do the needed changes |
actually taking a quick google it seems it is a url-loader/css-loader aka webpack bug webpack-contrib/css-loader#355 |
The other option I see is that the config format has changed for postcss |
@dbkr I updated the postcss to the newest version. Lets see if that helps. If you want you can test it. The CI seems to still work as before |
/me now does some testing and will maybe tommorow make a webpack 4 PR for the future ^^ but seperate from this one to have a clean pr |
ok I will test tommorrow. Setting up a fresh dev space after 600missing commits takes a lot of time. Installing deps takes on my hdd 1h. So I will have some commits to this tommorow propably |
@dbkr the last commit should have fixed your error now. The reason was you have scss inside a node_modules folder and I excluded that to speed up the build. I now included only the needed folders from node_modules to fix the failing to load and at the same time not to need to include the hole node_modules |
Tested it and it works for me. |
Some sizes for thoose who like to know them:
|
#6620 tracks an update to webpack4 |
This is optional PR which updates webpack to v3.x.x.
bundle.js size gets down to 8.4MB from about 9.0MB before and bundle.css changes from 3.3Kb to 12Kb.
(I personally think also it is slightly faster when building. Not saw any changes on the webpage in speed)