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

bundling is broken #68

Open
dimsmol opened this issue Sep 26, 2016 · 11 comments
Open

bundling is broken #68

dimsmol opened this issue Sep 26, 2016 · 11 comments

Comments

@dimsmol
Copy link

dimsmol commented Sep 26, 2016

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

ethul commented Oct 2, 2016

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

On Monday, 26 September 2016, Dmitry Smolin [email protected]
wrote:

purs-loader starts bundling right after the compilation

return bundle(options, cache).then(() => psModule)
,
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
cache.bundleModules.push(psModule.name)

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
#68, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAVYy-jwXlqrVnimRGUq1oKi2fuxCpKWks5quAk5gaJpZM4KG0iY
.

@ethul
Copy link
Owner

ethul commented Oct 12, 2016

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

dimsmol commented Oct 12, 2016

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

ethul commented Oct 12, 2016

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

@dimsmol
Copy link
Author

dimsmol commented Oct 13, 2016

Thank you!

@dimsmol
Copy link
Author

dimsmol commented Oct 14, 2016

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

@ethul
Copy link
Owner

ethul commented Oct 14, 2016

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

ethul commented Oct 14, 2016

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

@dimsmol
Copy link
Author

dimsmol commented Oct 15, 2016

  • We have to wait only for those that import PureScript files. But some module can import other module, which can then import PureScript. So, probably we need to wait for all in practice.
  • I'm not actually sure that bundling === undefined is a valid check. There is nothing in docs that saying it. I've looked at webpack code and tried several approaches and only this one seemed to work for me. However, I'm not sure how much it is correct and stable.
  • Unfortunately, I it's hard to provide simple use-case. In my situation JS code is huge and PureScript is small, that makes it compile and bundle earlier than all the JS modules are processed. But it also depends on the order of processing and who knows what else.

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:

  • More than one JS file should import PureScript (the first will start the compilation, some other should be late enough to get processed after it bundling is finished)
  • JS should import different PureScript modules (or else you won't see that one of them is missing in the resulting bundle)
  • PureScript modules should be small to compile quick enough (including all the dependencies)
  • Should somehow introduce enough delay between processing the first JS module importing PureScript and the last one. Making JS processing slow may not be enough because if they start in parallel, they can still finish in time to get their PureScript imports correctly bundled. It's also needed to either have a lot of JS modules to make them wait for processing (not be processed all in parallel at once) or modify webpack code to reduce parallelism.

@ethul
Copy link
Owner

ethul commented Oct 17, 2016

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

jbmusso commented Aug 15, 2019

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.

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