[wasm] Exclude node from the threaded version.

[annxingyuan]

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

---

This change is 

[annxingyuan]

Hey @tafsiri - I know I mentioned yesterday that I would try to implement a quick fix for adding Node support for threaded WASM yesterday, but after looking into it a bit more I think it makes more sense to exclude Node for now (more details in this issue: #3830). Let me know if you have any questions!

[tafsiri]

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @pyu10055, and @tafsiri)

---

tfjs-backend-wasm/package.json, line 39 at r1 (raw file):

  "browser": {
    "fs": false,
    "path": false,

are these still needed given your update to defer supporting threads in node?

---

tfjs-backend-wasm/rollup.config.js, line 64 at r1 (raw file):

      banner: PREAMBLE,
      sourcemap: true,
      globals: {'@tensorflow/tfjs-core': 'tf', 'fs': 'fs', 'path': 'path', 'worker_threads': 'worker_threads', 'perf_hooks': 'perf_hooks'},

Ditto with these updates.

[annxingyuan]

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055 and @tafsiri)

---

tfjs-backend-wasm/package.json, line 39 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

are these still needed given your update to defer supporting threads in node?

I think so, this means for a browser build we should not include the worker_threads / perf_hooks packages, which are only needed in node.

---

tfjs-backend-wasm/rollup.config.js, line 64 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Ditto with these updates.

I think so, and in general I feel like any exceptions we're making in rollup to packages like fs and path should apply to worker_threads and perf_hooks as well - does that seem right?

[JesseFarebro]

It would be nice to have this change upstream, without specifying these sinks bundlers get confused and will throw errors even for client-side packages. Right now I have to manually patch this for my deployments.

[tafsiri]

Could you give us more info on this? When you say manually patch, do you mean you need to add these to your bundler configuration explicitly (i.e. these are not picked up automatically). What kind of set up are you using?

[JesseFarebro]

You can see the issue by just using the WASM webpack starter. Update the tfjs version to 2.3 and try running the dev server. You'll see in the console that it fails on finding both worker_threads and perf_hooks.

By patch, I mean adding worker_threads and perf_hooks to the tfjs-wasm package.json. It doesn't seem to work if you add it to the root package.json.

[tafsiri]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @pyu10055)

---

tfjs-backend-wasm/package.json, line 39 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

I think so, this means for a browser build we should not include the worker_threads / perf_hooks packages, which are only needed in node.

Could you make an issue/doc documenting what happens (bug wise/behaviour wise) when these are not present (but with them specified as externals in our rollup config) , it would be nice to track that with specific errors to understand if there are alternatives or what benefits these provide. One benefit is that it does document node built ins that should be excluded by downstream users creating bundles themselves.

FYI since our convo about this i found this link https://github.com/defunctzombie/package-browser-field-spec which seems to describe this use/convention of the browser field (even though it is not in npm's official docs)

---

tfjs-backend-wasm/rollup.config.js, line 64 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

I think so, and in general I feel like any exceptions we're making in rollup to packages like fs and path should apply to worker_threads and perf_hooks as well - does that seem right?

Agreed. +1. In future we could look into automatically excluding all node built ins, i think i've seen some rollup plugins that list them all/make them easy to exclude.

[tafsiri]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @JesseFarebro, and @pyu10055)

---

tfjs-backend-wasm/package.json, line 39 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Could you make an issue/doc documenting what happens (bug wise/behaviour wise) when these are not present (but with them specified as externals in our rollup config) , it would be nice to track that with specific errors to understand if there are alternatives or what benefits these provide. One benefit is that it does document node built ins that should be excluded by downstream users creating bundles themselves.

FYI since our convo about this i found this link https://github.com/defunctzombie/package-browser-field-spec which seems to describe this use/convention of the browser field (even though it is not in npm's official docs)

Conversely/additionally it would be great to know what happens when these are present here but we remove them from the rollup config. It seems duplicative to have them in both places and i wonder if inertia is the main reason they are in both places.

[annxingyuan]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @JesseFarebro, @pyu10055, and @tafsiri)

---

tfjs-backend-wasm/package.json, line 39 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Conversely/additionally it would be great to know what happens when these are present here but we remove them from the rollup config. It seems duplicative to have them in both places and i wonder if inertia is the main reason they are in both places.

I did a little experimenting - it looks like putting these in versus taking them out does not have an effect on the process of building the WASM backend with rollup. However it looks like in the facemesh demo, I can avoid specifying worker_threads / perf_hooks in the facemesh demo's own package.json if I include them here!