ethul / purs-loader

PureScript loader for webpack
MIT License
186 stars 41 forks source link

bundling is broken #68

Open dimsmol opened 7 years ago

dimsmol commented 7 years ago

purs-loader starts bundling right after the compilation, but it does not guarantee that all the references to the PureScript modules are already collected. As a result, if compilation ends faster than modules processing, this code continues to add bundle modules that will never be bundled (because bundling is already started at this time).

I've got this problem compiling a large JS project with some of PureScript included, while compiling for the server side. For some reason bundling for client side either worked well or problems weren't obvious.

At least, I'd recommend to throw an error if more bundle modules are tried to be pushed when bundling is already started because else bundle is invalid anyway.

I don't see any easy way to fix the problem. It's needed to somehow wait for all the modules except PureScript ones to be processed before starting bundling. It can be done with checking _compilation.modules and not starting bundling unless all non-.purs of them have module.building equals to undefined (see this code). I couldn't find any better way to determine webpack have finished with the module. Anyway, it looks hacky.

Also, because all the modules must be ready before bundling, PureScript files cannot be used in loaders or as loaders with bundle: true mode. This looks pretty obvious, but probably should be documented. A workaround here can be to use special non-bundling settings for loaders, but haven't tried it myself yet.

ethul commented 7 years ago

Thanks for the report. Sorry for the delay. I will get back to this next week.

On Monday, 26 September 2016, Dmitry Smolin notifications@github.com wrote:

purs-loader starts bundling right after the compilation https://github.com/ethul/purs-loader/blob/896c11ece1c11c3a57c5169dbbaa24865016cded/src/Psc.js#L53, but it does not guarantee that all the references to the PureScript modules are already collected. As a result, if compilation ends faster than modules processing, this code https://github.com/ethul/purs-loader/blob/0b853815ef14d35cedebc2c7806fd2f9ff9d5ab5/src/index.js#L97 continues to add bundle modules that will never be bundled (because bundling is already started at this time).

I've got this problem compiling a large JS project with some of PureScript included, while compiling for the server side. For some reason bundling for client side either worked well or problems weren't obvious.

At least, I'd recommend to throw an error if more bundle modules are tried to be pushed when bundling is already started because else bundle is invalid anyway.

I don't see any easy way to fix the problem. It's needed to somehow wait for all the modules except PureScript ones to be processed before starting bundling. It can be done with checking _compilation.modules and not starting bundling unless all non-.purs of them have module.building equals to undefined (see this code https://github.com/webpack/webpack/blob/17ca14ccd96dcb951c29f6762260cb7a3fc434d0/lib/Compilation.js#L122). I couldn't find any better way to determine webpack have finished with the module. Anyway, it looks hacky.

Also, because all the modules must be ready before bundling, PureScript files cannot be used in loaders or as loaders with bundle: true mode. This looks pretty obvious, but probably should be documented. A workaround here can be to use special non-bundling settings for loaders, but haven't tried it myself yet.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ethul/purs-loader/issues/68, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVYy-jwXlqrVnimRGUq1oKi2fuxCpKWks5quAk5gaJpZM4KG0iY .

ethul commented 7 years ago

This definitely requires a bit more digging on my part. However, I wonder if it may be worth evaluating the pros/cons of keeping the bundling feature. If you don't mind my asking, do you see a significant improvement in the end result of the JS in a production build from webpack with bundling enabled vs. disabled?

dimsmol commented 7 years ago

Yes, it's significant. Especially if you are careful and do not reference JS modules from PureScript code. It reduces code size a lot.

ethul commented 7 years ago

Gotcha. I will look into a possible solution to this.

dimsmol commented 7 years ago

Thank you!

dimsmol commented 7 years ago

here is quick and dirty fix I'm using right now (just to demonstrate possible fix idea): https://github.com/ethul/purs-loader/compare/master...grammarly:ac7aba1 (plz ignore added lib/* files, only 2 src files at the very end were actually changed)

ethul commented 7 years ago

Thanks for the diff. Just so I understand, is it that we have to wait for all of the non-PureScript modules in general to have bundling === undefined? Or is it that we have to wait for all non-PureScript modules that import PureScript files to have bundling === undefined?

ethul commented 7 years ago

Also, you don't happen to have a small use-case that reproduces this issue, do you?

dimsmol commented 7 years ago

Potentially, it could be reproduced by adding artificial huge delay to JS modules processing (add setTimeout or something to JS loader) and having several JS modules to import very small PureScript ones. I will try to create sample project with this, but probably you can do it faster by just modifying JS loader directly in node_modules. The key factors to reproduce:

ethul commented 7 years ago

Thank you for the details on how to reproduce this. I am wondering if there might be a webpack plugin hook that we can leverage in the loader to run psc-bundle at the appropriate time. I thought optimize-tree might be a possibility, but it seems a loader can't hook into that soon enough. But maybe there is a way. I still have to explore this a bit.

Another hook that might work is if we can use the build-module, succeed-module, and failed-module hooks. For instance, we can store a promise for each (non-PureScript) build-module and then resolve/reject it accordingly in the succeed-module and failed-module hooks, respectively. Then once all promises are resolved, do the bundling. However, it seems a loader might be able to hook on to these soon enough.

jbmusso commented 5 years ago

I may be experiencing a comparable issue, and using bundle: true solves it -- this could be fixing a symptom only. When bundle option is set to the default value, purs-loader throws TypeError: Cannot read property 'imports' of undefined. The error first arises when using the react-basic package, and creating a very simple component creates a TypeError: React_Basic.createComponent is not a function error.