Don't fail the build when wasm modules are used. #410
Conversation
…in basic-with-wasm-module/main.js is accepted by eslint. This also removes unnecessary path globbing from the eslint invocation because it was failing on Windows with the new eslint version (not sure why) - but it's not necessary anyway because eslint recurses by default and only checks .js files by default.
This test uses the minimumWebpackMajorVersion flag so that we don't attempt to compile with webpack 2 or 3 which don't understand wasm modules.
mstange
force-pushed
the
fix-wasm
branch
2 times, most recently
from
2243f8f to
ea905a5
Compare
|
Hey. Sorry for a delay. This is great that you did this. But |
|
The patch touches |
|
Thanks for taking a look! |
|
Oh, yeah, indeed, there are changes in |
|
hey @NekR, could you look at it ? :) |
| } | ||
| }; | ||
|
|
||
| if (webpackMajorVersion === '4') { |
There was a problem hiding this comment.
should probably use if (parseInt(webpackMajorVersion, 10) >= 4) to be future proof.
| parseInt(webpackMajorVersion, 10) < testFlags.minimumWebpackMajorVersion) { | ||
| throw new Error('unsupported webpack version'); | ||
| } | ||
| delete testFlags.minimumWebpackMajorVersion; |
There was a problem hiding this comment.
How come you are deleting this? If set, would you not expect it to be present further down the execution flow? Maybe I am missunderstanding something :)
|
|
||
| var hash = _loaderUtils2['default'].getHashDigest(compilation.assets[key].source(), 'sha1'); | ||
| var source = compilation.assets[key].source(); | ||
| if (source instanceof ArrayBuffer) { |
There was a problem hiding this comment.
Do you know of any other situation that could cause source to be an ArrayBuffer? (It shouldn't matter though, since the same logic should apply.)
|
@mstange I approved this with some minor comments, which you could look at. It would still require @NekR's approval before merge though @NekR - the actual change in this PR is very minor and should not affect non-webassembly users, so I think it would be worth merging this. It's backwards compatible, so should fit in a patch / minor version |
|
@GGAlanSmithee I think I can merge this. |
|
Hmm, it fails CI after merge from master. Probably my or other PR changes did something to it. |
|
Okay, it seems to be because of newer webpack version. I'll resolve that locally. @mstange thanks for contributing this. It's very much appreciated. Great work 👍 @GGAlanSmithee thanks for taking care of this. It wouldn't be possible to merge and all this PRs right without you help 💪 👍 |
4 participants
Fixes #408.
The fix itself is straightforward but adding a test was a bit more troublesome. I had to upgrade eslint and add babel-eslint so that the dynamic
import()syntax would pass eslint. I also tweaked the harness a bit to log more details when things fail. The test only has a reference for webpack4 and is skpped on earlier webpack versions.