Skip to content

Conversation

gribnoysup
Copy link
Collaborator

@gribnoysup gribnoysup commented Jan 28, 2021

This PR refactors node-runtime-worker-thread bundling process to not inline worker source and keep the package as external/peer dependency in the bundle of compass-shell. I spent some time digging into bundling of electron apps and this approach is something that I somehow completely missed as possible during the initial implementation.

Benefits of this approach is a simplified build pipeline for the library itself, no need to maintain a custom webpack loader and in theory this should unblock (or definitely make much easier) the inclusion of the interruptor with the library eventually.

Downside is a bit cumbersome way of importing worker source in the library that relies on the files staying close together in one place and the need to keep in mind that the runtime is now a peer dependency of the compass shell, but both are pretty small downsides in my opinion compared to the simplification of the whole thing.

(The PR base branch is #572 to avoid conflicts between the two if branched from master)

Base automatically changed from compass-4557-advanced-serialization to master January 29, 2021 07:50
This allows us to avoid inlining source of node-runtime-worker-thread when
used in compass, but has a small downside of manually installing runtime in
compass main repo
@gribnoysup gribnoysup force-pushed the node-runtime-no-more-inlines branch from d2bfed2 to ba2eef3 Compare January 29, 2021 07:56
Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This LGTM, although I have to admit that I'm not fully clear on how the move from a regular dependency to a peer dependency comes into play here (the compass-shell package is still the only consumer of node-runtime-worker-thread, right?). If you want, feel free to enlighten me, but either way I know that you have thought this through :)

@gribnoysup
Copy link
Collaborator Author

Ah, that's a good point that it's not clear. So the reason for this is that by default when library is a peer dependency compass plugin in "production" mode will exclude it from the bundle (I think it's part of the default compass plugin template, not only a compass-shell thing):

// Auto-create webpack externals for any dependency listed as a peerDependency in package.json
// so that the external vendor JavaScript is not part of our compiled bundle
new PeerDepsExternalsPlugin(),

I was initially thinking that if this externals plugin is already there, I can move the dep to peer deps and be done with it, eventually I had to manually apply the externals exception in the "dev" config also. You're right that it will be only used by compass-shell and thinking this over again, I'm leaning to moving it back to deps and making an explicit externals record for the node-runtime-worker-thread in the "base" config

@addaleax
Copy link
Collaborator

I'm leaning to moving it back to deps and making an explicit externals record for the node-runtime-worker-thread in the "base" config

Okay, if that works then I think that sounds a bit more straightforward, but in the end, the important thing is that it works :)

@gribnoysup
Copy link
Collaborator Author

Okay, moved it back to deps and moved the externals option to base config. Will merge without waiting for ci as nothing really changed and previous run passed

@gribnoysup gribnoysup merged commit 18d6b9a into master Feb 1, 2021
@gribnoysup gribnoysup deleted the node-runtime-no-more-inlines branch February 1, 2021 09:44
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.

2 participants