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

Documentation about parcel build is deprecated ? + tree-shaking not working #232

Open
PacoDu opened this issue Apr 13, 2020 · 6 comments
Open
Labels

Comments

@PacoDu
Copy link

PacoDu commented Apr 13, 2020

Hi, I did some tests to package a library that depends on threads.js and is built with parcel-bundler.

For compatibility purposes with NodeJS and browsers the library is written in ES6 then compiled in CJS for NodeJS and bundled for CDN delivery.
What I discovered is that the recommended parcel import to register threads.js (described here) doesn't have any effect, and the library is working as expected without the import 'threads/register'. It also prevents users to properly build their application if they want to use webpack.

The CDN delivery method is not working as we can't import the worker as parcel automatically split the worker code in a different file.

I've created a test repository https://github.com/PacoDu/threads-package that defines a simple library using threads.js and implement multiple client softwares using this library.

Also the --experimental-scope-hoisting (tree-shaking) option of parcel is not working with threads.js it throws:

../node_modules/threads/dist-esm/worker/implementation.browser.js does not export 'default'

Maybe I'm doing this the wrong way. Do you have any recommendation for packaging threads.js in a library ? (maybe it should be a peerDependencies ?)

@andywer
Copy link
Owner

andywer commented Apr 14, 2020

Hey @PacoDu!
You seem to have spent a great amount of effort on this investigation (great stuff 👍), but since it’s a lot of content I will try to tackle one point at a time.

Let’s start with the easy bit:
There have been only so many attempts to use threads.js in a library so far and the best approach is not completely clear right now.

There is already some (a bit implicit) discussion in #211. Spawning workers from blobs bundled with the library might be a promising approach in the future.

@andywer
Copy link
Owner

andywer commented Apr 14, 2020

What I discovered is that the recommended parcel import to register threads.js (described here) doesn't have any effect, and the library is working as expected without the import 'threads/register'.

Have you only tried building it or have you also tried running the resulting bundles?

The issue with Parcel has always been that it detects new Worker() instantiations automatically, but only if Worker references a global, not if it references an import (!).

Parcel would still produce an output bundle, but it would have failed to create a second bundle for the worker on import { Worker } from "threads" if I recall correctly.

Didn’t have time to actually run your test setup yet, though.

@PacoDu
Copy link
Author

PacoDu commented Apr 14, 2020

Hey !
Yes sorry, I've mixed multiples issues here 😅

Have you only tried building it or have you also tried running the resulting bundles?

Yes, every main-* sub-project in the repository has been run successfully. I'll see if I can find some time to add tests to the demo repo.

Parcel would still produce an output bundle, but it would have failed to create a second bundle for the worker on import { Worker } from "threads" if I recall correctly.

It doesn't seem to be the case with the current version of parcel-bundler, I'll add a CI to demonstrate that the build is passing and produces a separate worker bundle without import 'threads/register'

There is already some (a bit implicit) discussion in #211. Spawning workers from blobs bundled with the library might be a promising approach in the future.

Thanks for the related issue, this is indeed possible with webpack but, to my knowledge, not with parcel 😟. Maybe it will be possible in parcel v2, not sure tho.

@andywer
Copy link
Owner

andywer commented Apr 18, 2020

@PacoDu Can we close this issue? Tbh, I lost the overview here… 😉

@PacoDu
Copy link
Author

PacoDu commented Apr 18, 2020

@PacoDu Can we close this issue? Tbh, I lost the overview here… 😉

But it's not resolved, if you want I can create 2 issues: one about the tree shaking not working on parcel and an other one about the import 'threads/register that doesn't seem required anymore.

@andywer
Copy link
Owner

andywer commented Apr 18, 2020

That sounds like a good idea 👍

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

No branches or pull requests

2 participants