search

NekR / offline-plugin

Offline plugin (ServiceWorker, AppCache) for webpack (https://webpack.js.org/)
MIT License
4.52k stars 295 forks source link

Don't fail the build when wasm modules are used. #410

Closed mstange closed 5 years ago

mstange commented 6 years ago

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.

NekR commented 6 years ago

Hey. Sorry for a delay. This is great that you did this. But lib/ is generated folder. You need to modify files in src/ and then generate lib/ folder. See https://github.com/NekR/offline-plugin/blob/master/CONTRIBUTING.md for details.

mstange commented 6 years ago

The patch touches src/ too. If I recall correctly, I generated the change to lib/ the way you say (by building), but it's been a while and I don't 100% remember. I guess it's possible that I edited lib/index.js manually with the equivalent change. I can build again to make sure, if you'd like.

mstange commented 6 years ago

Thanks for taking a look!

NekR commented 6 years ago

Oh, yeah, indeed, there are changes in src/ as well. My bad. I'll take a closer look at it soon 👌

julienw commented 6 years ago

hey @nekr, could you look at it ? :)

GGAlanSmithee commented 5 years ago

@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

NekR commented 5 years ago

@GGAlanSmithee I think I can merge this.

NekR commented 5 years ago

Hmm, it fails CI after merge from master. Probably my or other PR changes did something to it.

NekR commented 5 years ago

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 💪 👍