fix issue with @loadable/webpack-plugin not working in production mode and SSR

[tadeuszwojcik]

This is my stab at #179 .

I've added path option to the webpack plugin, that allows specifying where loadable-stats are written to when writeToDisk option is set to true. This way webpack assets compiler always use filename and not absolute path when adding loadable stats to the assets. If path is not set by default loadable-stats are written to output.path set in webpack config.

I also fixed issue I had with eval('module.require') trick as it was throwing errors (eval is not a function). I've replaced it with __non_webpack_require__ and that seems to be working fine.

I've tested it with razzle example in the repo and it works fine both in dev and production mode (after yarn build command).

Closes #179

[gregberge]

I am sorry but it does not work. Your path option is only used if writeToDisk is set to true. It is confusing. All webpack assets are generated into outputPath, so what should we specify another path for this specific file? I think the path option is not relevant.

[gregberge]

Are you sure that it will not cause any error or warning when built using webpack?

[tadeuszwojcik]

Yes, tried that with razzle that uses webpack for the server bundle as well

[gregberge]

@tadeuszwojcik thanks for your work, but it is important to keep it consistent and compliant with webpack ecosystem.

[tadeuszwojcik]

@neoziro thanks for taking the time and looking into this, I've used the path in the same way that https://github.com/ztoben/assets-webpack-plugin/blob/master/readme.md uses, happy to continue this conversation after Christmas as I'm on holidays right now. Please don't close/rejected it before then, I'll try to put more details about rationale of this pull request and why it's essential for razzle and razzle like setups! Thanks!

[gregberge]

@tadeuszwojcik OK, if assets-webpack-plugin does it, it is probably a good thing. Maybe we could just always write it to disk, so the path option would not be confusing. Happy holidays, we can talk about it next year!

[tutok]

@neoziro What is the status of this issue? Is there anything that you would like to change and/or you are not sure about? Maybe I can help somehow?

From my perspective changes done by @tadeuszwojcik are ok. Actually, many webpack loaders/plugins follow that pattern were output path can be overridden, e.g. file-loader

[gregberge]

@tutok my concern is that is an absolute output path is specified it will be written in two different folders: https://github.com/smooth-code/loadable-components/pull/187/files#diff-567a0d765f93649fdd2c856a649ba64bR29

I think it is not good. This is the only blocking point.

[dlebedynskyi]

@tadeuszwojcik @neoziro
When I wrote original version it was expected that when using writeToDisk, filename must be an absolute path.
This was done for

• webpack dev server to actually write file on file system.
• most cases you don't want to output assets file at same level as you browser assets. kinda extra security level.

currently we are using webpack dev server for client bundle and output is

build/
- assets.json 
- /dist/ 
  - client.bundle.js

where asset.json absolute build path.

we can check this logic that I used as reference back then.

[gregberge]

With the actual code, stats will be written two times if writeToDisk is set to true. It is not good for me. I think we have to change the API:

new LoadablePlugin({ writeToDisk: { filename: '/your/custom/path/loadable-stats.json' } })

This way it is clear that the asset will be written to disk at a specific location. We can also add a config option outputAsset that will control if a new asset is added or not.

What do you think?

[tutok]

@neoziro you mean writeToDisk could be {filename: string} or boolean right? for me this is ok.

btw. is even loadable-stats.json an webpack asset? Maybe this file should not be considered as webpack asset as it contains rather metadata about them. Moreover loadable-stats.json is not event used by "client" build, it is especially created for "server" build for SSR. (And actually it explain why it is good to not keep this file inside "client" output directory)

[gregberge]

@tutok yeah this is what I mean for writeToDisk option. I understand your point but I think it is simpler by default to emit it from webpack assets. Also in a specific configuration people could want it to not be written on the disk (SSR).

[tadeuszwojcik]

@neoziro I've pushed the change with writeToDisk {filename: string} change, hopefully it will make webpack-plugin API more self explaining.

[gregberge]

I am sorry, I would like you to make one last change to make it easy for users:

• writeToDisk can be set to true, false or { filename: "..." }, it is false by default
• false will not write to disk (default value)
• true will write to disk using filename (exactly like today)
• { filename: "..." } will write to disk to a custom location

Can you also add a outputAsset option?

• outputAsset can be set to true
• it is true by default
• true will append stats into assets (like today)
• false will not append stats into assets

This way we could do:

new LoadablePlugin({ outputAsset: false, writeToDisk: { filename: 'my/custom/stats.json' })

I think the API is great and can handle all cases.

[dlebedynskyi]

@neoziro I don't think you last comment was for this issue. Probably should not close this ticket either

[gregberge]

Oups sorry!

[gregberge]

@tadeuszwojcik any update?

[tadeuszwojcik]

@neoziro sorry for delay, I've updated plugin with new API as requested, hope it's fine now. I didn't have time for updating docs yet, so hopefully someone else would be able to cover that. Thanks!

[tutok]

@neoziro can we merge?

[gregberge]

Thanks for your work!