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

Filer Webpack Plugin #771

Merged
merged 29 commits into from
May 26, 2021
Merged

Filer Webpack Plugin #771

merged 29 commits into from
May 26, 2021

Conversation

bcheidemann
Copy link
Contributor

@bcheidemann bcheidemann commented Apr 4, 2021

Closes #772

TODO:

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

@bcheidemann bcheidemann changed the title Filer Webpack Plugin WIP: Filer Webpack Plugin Apr 4, 2021
@bcheidemann
Copy link
Contributor Author

@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
Copy link
Contributor

humphd commented Apr 6, 2021

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
Copy link
Contributor Author

@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.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/webpack-plugin/plugin.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tests/spec/webpack-plugin/webpack-plugin.spec.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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 bcheidemann changed the title WIP: Filer Webpack Plugin Filer Webpack Plugin Apr 18, 2021
@bcheidemann
Copy link
Contributor Author

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
Copy link
Contributor

humphd commented May 26, 2021

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

@humphd humphd merged commit b3caddc into filerjs:master May 26, 2021
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.

path module not shimmed correctly
3 participants