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

Add option to emit file in @loadble/webpack-plugin #158

Closed
marcneander opened this issue Nov 20, 2018 · 7 comments
Closed

Add option to emit file in @loadble/webpack-plugin #158

marcneander opened this issue Nov 20, 2018 · 7 comments

Comments

@marcneander
Copy link
Contributor

marcneander commented Nov 20, 2018

🚀 Feature Proposal

Using @loadable in server rendered apps using webpack-dev-server that keeps output in memory is not working very well right now. One way to solve this would be to have an option to emit the file to disk.

I don't have any knowledge regarding the webpack API and if it's safe or not to do this. However I know there are atleast two webpack plugins that does this today already.

Motivation

I think think library is awesome and I've spent a couple of days implementing @loadable into our frontend platform. It works great but we also were in the process of switching build-system to razzle and that's where I ran into a wall.

Razzle runs two instances of webpack (one for the server and one for the client). The client side of webpack generates the loadable-stats.json file and is kept in memory during development (razzle start). It's currently very hard for the server to get a hold of this file...

Example

I've forked the @loadable/webpack-plugin and made some quick changes that seems to work well for us. As previously stated, I don't know the implications of doing it like I have. I would need to get familiar with the webpack plugins API to tackle this issue.

Here's a gist of the changes I've done: https://gist.github.com/marcneander/2bddd49fd3bd8c6065d8e51b221168a4

Right now I'm not capable of doing much coding outside of work. I might be able to convince my manager to let me spend a few hours on this.

@dlebedynskyi
Copy link
Contributor

dlebedynskyi commented Nov 20, 2018

Was just creating exact same thing https://gist.github.com/dlebedynskyi/7fd345f3d193bb78790c0e122e23012c

a bit more checks based on https://github.com/themgoncalves/react-loadable-ssr-addon/blob/master/source/ReactLoadableSSRAddon.js

also switch to emitAsync to handle error during file write - like EACCESS etc. Otherwise errors are swallowed

@marcneander
Copy link
Contributor Author

@dlebedynskyi Great! Only thing I might see as a problem is the if statement around outputing the file if it already exists. Might be best to always output the file? I don't think we can assume everyone does full cleanups.

@neoziro Would you consider a PR regarding this?

@gregberge
Copy link
Owner

Thanks both for your work! Two forks prove that the feature should be included in the plugin.

I think we can always write the file, as other plugins you mentioned. Is there a use-case where we wouldn’t want to write it on the disk?

I am not a Webpack expert, so I don’t know the best way to do it.

Anyway I am open to PR, when the API will be finalized and decided. Questions:

  • Should we always write it in the disk?
  • If there is some use cases to not write it on the disk, what are they?
  • Is it possible to automatically detects these use cases?
  • If not, should we add an option to disable writting? What would be the default?

@dlebedynskyi
Copy link
Contributor

@marcneander I mostly copied code from react-lodable-ssr-addon. we can check for file already exist. I don't think it will be a problem anyways.

@neoziro I think behavior as is, don't write to disk by default is fine.
We can add just option for writeToDisk (default: false) and do that by user request. This is way that webpack-assets-manifest does it.

Imo:

 new LoadablePlugin({writeToDisk: true, filename: path.resolve(absoluteFilePath)})

work fine.

Not much of webpack expert myself.
But have been digging quite a bit throw lodable-* packages. I can open PR if needed.

@marcneander
Copy link
Contributor Author

marcneander commented Nov 21, 2018

@dlebedynskyi Atleast when using razzle. If we don't write the file even though it already exists we will get bugs if we do razzle build before razzle start. Since razzle start won't cleanup build directory but would use the loadable-stats.json file from razzle build output.

Please go ahead with a PR! If you need any help, please let me know 👍

@dlebedynskyi
Copy link
Contributor

dlebedynskyi commented Nov 21, 2018

Open PR #161

@marcneander I tested without clean up on our build process and have not seen any issues around writing over old file after build before start.

Please feel free to test on your razzle setup for any issues.

@gregberge
Copy link
Owner

Already done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants