-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Webpack no longer supports define correctly #5316
Comments
yep this is a problem... |
/cc @davidschwegler problem still exists in |
Hi @evilebottnawi I've retried today using webpack 4.8.3 and can confirm the issue with requirejs/ amdefine still exists. |
@evilebottnawi Looking through the issue it looks like a minimal reproduction example is lacking. Would it be helpful if I provided one? |
*rips out webpack and switches to browserify |
/cc @ooflorent |
@sverweij Absolutely. Any simple repro would be appreciated. |
I've found the issue. Problem is that tl;dr: |
@ooflorent some old libraries can contain this code, it is not always possible to remove it |
@evilebottnawi I understand but this is not a webpack bug. webpack fixed a bug that broke existing libraries because they were doing broken things. |
A fix would be to ignore the redefinition of |
@ooflorent looks like we can close issue here, but @sokra add bug labels, maybe he have idea how solve this without break code |
If this cannot be solved without breaking (t.b.c.) it might be useful to document any known workarounds for anyone bumping into this (I can help). @ooflorent seeing you found the root cause: still interested in a simple repro? |
@sverweij no, thank you. I was able to create a repro to analyze what was going on 👍 |
This issue had no activity for at least half a year. It's subject to automatic issue closing if there is no activity in the next 15 days. |
Issue was closed because of inactivity. If you think this is still a valid issue, please file a new issue with additional information. |
We already proved that compressjs works as a bz2 decoder in the rosbag.js tests: https://github.com/cruise-automation/rosbag.js/blob/68526faa242bb76943b6e42fa452f1eadb6cbc73/src/bag.test.js#L175-L178 But it wasn't imported in webviz, so bz2-compressed bags (such as the ones at https://vision.in.tum.de/data/datasets/rgbd-dataset/download#freiburg3_cabinet) didn't work by default. It was almost very straightforward to add, except that a webpack/AMD module issue (webpack/webpack#5316) prevents us from actually using compressjs in our webpack build. I worked around it by using a string-replace-loader to just remove the offending line. It's a pretty horrible hack, but it works...let me know if you have any better suggestions.
Do you want to request a feature or report a bug?
Bug
What is the current behavior?
Webpack no longer appears to support define correctly.
If the current behavior is a bug, please provide the steps to reproduce.
We upgraded from webpack 2.2.1 to 2.6.1. In 2.2.1 everything works, but in 2.6.1 it doesn't.
compressjs
module, which uses define like so (see https://github.com/cscott/compressjs/blob/master/main.js)Attempted workarounds
I have tried excluding the module from webpack via noParse (though the docs say not to exclude anything that uses define or require):
At runtime, this results in a different error:
I have tried additionally adding an alias:
Offending webpack code
It appears the webpack code in Parser.js no longer returns a value for the define expression. In 2.2.1, parser returned a BasicEvaluatedExpression, but as of 2.6.1 it returns nothing, because the "define" definition has been pushed onto the scope for some reason.
this.scope.definitions
contains 'define' now, whereas it was previously empty.I'm guessing this is due to the prewalking that was introduced starting in 2.4.0 here.
What is the expected behavior?
Modules such as this runs without errors.
If this is a feature request, what is motivation or use case for changing the behavior?
Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.
Testing using the latest Chrome version.
The text was updated successfully, but these errors were encountered: