bcheidemann commented 3 years ago

Closes #772

TODO:

[x] Write tests for the plugin
[x] Manual tests & run webpack output in browser to make sure it all works properly

bcheidemann commented 3 years ago

@humphd while I was working on this I saw that the Buffer exported in filer seems to be the global Buffer object. I'm wondering therefore if I should remove it from the plugin as it would just be shimming it with itself. Maybe I missing something here?

humphd commented 3 years ago

I think that's fine, it will use https://github.com/webpack/node-libs-browser already in our builds, so webpack should do the same?

bcheidemann commented 3 years ago

@humphd As discussed I have removed the option to shim buffer and added a brief comment to the README to explain why buffer is not shimmed by the FilerWebpackPlugin and how to handle it. I've also written tests for the FilerWebpackPlugin so this should be more or less ready to review - though I haven't done a full end to end test using it in a test project so there's still a possibility that I will add some more commits if I find any issues (though I think this is unlikely). Either way, I'll let you know when I've finished the end to end tests.

codecov-commenter commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@3e88aec). Click here to learn what that means. The diff coverage is 98.80%.

@@            Coverage Diff            @@
##             master     #771   +/-   ##
=========================================
  Coverage          ?   87.44%           
=========================================
  Files             ?       23           
  Lines             ?     1824           
  Branches          ?        0           
=========================================
  Hits              ?     1595           
  Misses            ?      229           
  Partials          ?        0

Impacted Files	Coverage Δ
src/index.js	`100.00% <ø> (ø)`
shims/fs.js	`95.83% <95.83%> (ø)`
shims/path.js	`100.00% <100.00%> (ø)`
shims/providers/default.js	`100.00% <100.00%> (ø)`
src/webpack-plugin/index.js	`100.00% <100.00%> (ø)`
src/webpack-plugin/processors.js	`100.00% <100.00%> (ø)`
src/webpack-plugin/schema.js	`100.00% <100.00%> (ø)`
src/webpack-plugin/utils.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3e88aec...9fb6596. Read the comment docs.

bcheidemann commented 3 years ago

@humphd I patched a couple of minor issues that came up when I was testing the plugin in a test project but if you're happy with those commits then this should be good to go.

bcheidemann commented 3 years ago

I think this looks really good, great work. The docs and tests are appreciated. I agree that adding an e2e test that uses this somehow would be good in a follow-up.

I'm a little leery of adding these new dependencies, and further bloating the final bundle. We could split this out into a separate repo under the filerjs org, or could add a second build step to build the webpack plugin separately from Filer, making it an optional extra thing one could import on demand. Either of those could happen as a follow-up optimization vs. blocking this further.

I'm inclined to take this once you finish the fixes, and we iterate from there.

I'd be happy to move this out into a separate repo. If we go down that route then I will wait to add full e2e tests for the plugin until it's been moved to the other repo if that's okay.

bcheidemann commented 3 years ago

Hey @humphd,

Been super busy these past few weeks so not had time to do anything on filer. Just come back now as I have a few hours free and don't want open PRs etc to go stale. Was wondering if there was a reason why this wasn't merged yet? Happy to make any changes needed =)

All the best,

Ben

humphd commented 3 years ago

Hey, apologies for the delay. I'll merge now.

filerjs / filer

Filer Webpack Plugin #771

Codecov Report