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

WIP: Bump webpack to v5 (fixes #2050) #2066

Closed
wants to merge 2 commits into from
Closed

WIP: Bump webpack to v5 (fixes #2050) #2066

wants to merge 2 commits into from

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Dec 20, 2020

Imports are required to be done a little differently, and there needs to be a "type: module" in the package.json

@josdejong
Copy link
Owner

josdejong commented Dec 23, 2020

Thanks a lot Flaviu! I'm happily surprised that there are no config changes needed for webpack, looks quite straightforward.

A few small remarks:

  1. I think the old treeShakingApp.js can be removed since you copied it to ./source.
  2. I think it will be wise not to switch to karma-webpack@5.0.0-alpha.5 since it's an (unstable) alpha version, shall we await upgrading it until v5.0.0 is published for real?

EDIT: or is the upgrade of karma needed to get the integration tests running?

@flaviut
Copy link
Contributor Author

flaviut commented Dec 23, 2020

I think the old treeShakingApp.js can be removed since you copied it to ./source.

It was, I think the github ui just didn't do a very good job making it clear.

EDIT: or is the upgrade of karma needed to get the integration tests running?

I went through and bumped everything webpack related up--but it turns out that karma-webpack doesn't need to be updated yet, unless you're experiencing these bugs. I've brought it back to v4, and test:all still works fine for me now.

@josdejong
Copy link
Owner

It was, I think the github ui just didn't do a very good job making it clear.

Ah, now I see, you're right

I see all except one of the CI tests are green. Only "Browser testing locally / browser-tests (pull_request)" gives an error. The logs say:

Cannot find plugin "/home/runner/work/mathjs/mathjs/node_modules/karma-webpack".

Which is just odd. When I run npm run test:browser locally I get an error which makes more sense:

> npm run test:browser

24 12 2020 08:51:57.789:ERROR [plugin]: Error during loading "...\mathjs\node_modules/karma-webpack" plugin:
  Cannot find module 'webpack/lib/dependencies/SingleEntryDependency'

Any ideas?

@josdejong
Copy link
Owner

Maybe we're hitting this issue: codymikol/karma-webpack#431

@flaviut flaviut changed the title Bump webpack to v5 (fixes #2050) WIP: Bump webpack to v5 (fixes #2050) Feb 4, 2021
@josdejong
Copy link
Owner

I see some new commits here @flaviut , are you picking this up again?

Pinging @JayFate, please be aware @flaviut was also working on this (see #2094)

@flaviut
Copy link
Contributor Author

flaviut commented Feb 7, 2021

Yup, I've had some free time lately. No promises though--fixing issues arising from dependencies 5 levels down gets frustrating quickly :)

@josdejong
Copy link
Owner

Thanks @flaviut. Yes this stuff can be very frustrating indeed!!

@josdejong
Copy link
Owner

@flaviut just to be sure you're aware of it: you can run those browser tests locally via:

npm run test:browser

@flaviut
Copy link
Contributor Author

flaviut commented Feb 7, 2021 via email

Imports are required to be done a little differently, and there needs to
be a "type: module" in the package.json
@flaviut flaviut mentioned this pull request Sep 15, 2021
@gwhitney
Copy link
Collaborator

This appears to be superseded by a later PR, as mathjs@11.0.1 does use webpack 5. If you agree, feel free to close or let me know to.

@gwhitney gwhitney mentioned this pull request Aug 17, 2022
@josdejong
Copy link
Owner

Yes 🎉 this has been addressed (it required to drop IE11 support)

@josdejong josdejong closed this Aug 17, 2022
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.

None yet

3 participants