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

Natural (stemming dependency) crashes in browser #31

Closed
micheleriva opened this issue Jul 19, 2022 · 4 comments
Closed

Natural (stemming dependency) crashes in browser #31

micheleriva opened this issue Jul 19, 2022 · 4 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed v0.1.0
Projects

Comments

@micheleriva
Copy link
Member

Describe the bug
When running Lyra in a browser, you will get the following error:

chunk-OROXOI2D.js?v=4fc34af8:10 Uncaught Error: Dynamic require of "webworker-threads" is not supported
    at chunk-OROXOI2D.js?v=4fc34af8:10:9
    at ../../../node_modules/.pnpm/natural@5.2.2/node_modules/natural/lib/natural/classifiers/classifier_train_parallel.js (classifier_train_parallel.js:6:13)
    at __require2 (chunk-OROXOI2D.js?v=4fc34af8:16:50)
    at ../../../node_modules/.pnpm/natural@5.2.2/node_modules/natural/lib/natural/classifiers/classifier.js (classifier.js:28:25)
    at __require2 (chunk-OROXOI2D.js?v=4fc34af8:16:50)
    at ../../../node_modules/.pnpm/natural@5.2.2/node_modules/natural/lib/natural/classifiers/bayes_classifier.js (bayes_classifier.js:26:20)
    at __require2 (chunk-OROXOI2D.js?v=4fc34af8:16:50)
    at ../../../node_modules/.pnpm/natural@5.2.2/node_modules/natural/lib/natural/classifiers/index.js (index.js:25:27)
    at __require2 (chunk-OROXOI2D.js?v=4fc34af8:16:50)
    at ../../../node_modules/.pnpm/natural@5.2.2/node_modules/natural/lib/natural/index.js (index.js:37:3)

To Reproduce
Steps to reproduce the behavior:

  1. Clone the Lyra repository
  2. Go inside the packages/examples/with-react directory
  3. Run the development server with pnpm dev
  4. Go to http://localhost:3000 and open the development console

Expected behavior
Lyra should work out of the box on browsers

Screenshots
Screenshot 2022-07-19 at 10 28 48

Desktop (please complete the following information):

  • OS: OSX Monterey
  • Browser Chrome
  • Version latest
@micheleriva micheleriva added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed v0.1.0 labels Jul 19, 2022
@micheleriva
Copy link
Member Author

Possibly related to NaturalNode/natural#368

@micheleriva micheleriva added this to To do in Lyra v0.1.0 Jul 19, 2022
@silawrenc
Copy link

I've been taking a look at this for a few hours now, I think there's a bunch of things going on here:

  • vite doesn't support require commonjs-style module resolution Unlike e.g. webpack <=4 which ultimately treats everything (imports or requires) with the semantics of commonjs. Hence why things like https://www.npmjs.com/package/@originjs/vite-plugin-commonjs exist. But even those can only convert a dynamic commonjs require syntax into a static esm one via an AST transform, they can't recreate the behaviour of synchronous dynamic imports. So the error being thrown can be sidestepped by installing a vite plugin like the one above, but all that does is change the problem, because now we have a static (hoisted) import for an optional package with no guard round it. We could just install webworkers-threads as a direct dep of the example (or the lib) however...
  • ...webworkers-threads definitely doesn't work in the browser. With most new build tools it would be possible to setup an alias/stub which just threw an appropriate error (aka one that was caugh on this line to "fix" this, but that would be build tool setup each time to consume this package.

So I think the tl;dr is that this library won't "just work" on web for newer build tools, because natural won't. I think it would be pretty easy to get this working with e.g. webpack v4 (although I haven't confirmed this) but that's not much use as the whole FE ecosystem is already moved on from that. Alternatively it could be worth opening an issue on the natural repo to see if they'd be interested in updating their check for threading support, or even using web workers if they're available natively (e.g. in browser, deno, electron environments) as it looks like the library they're using is modelled on the web worker standard anyway.

@simoneb
Copy link
Contributor

simoneb commented Jul 19, 2022

Upon a quick look, I don't think Natural works in the browser, see the many issues closed on the topic.

This specific error can be easily bypassed by configuring vite in this way so it ignores dynamic imports, but once you do that you'll see other errors coming up due to Natural using many Node-specific APIs which I'm not sure how easy or feasible it is to
support in a browser environment.

export default defineConfig({
  plugins: [react(), Unocss({})],
  build: {
    target: ["es2020"],
    commonjsOptions: {
      ignoreDynamicRequires: true
    }
  },
});

@micheleriva
Copy link
Member Author

Thank you @silawrenc and @simoneb, this analysis is crystal clear. Natural is becoming more and more complex to maintain in this codebase, and this issue specifically is a blocker. So I'll close this issue and open a new one to migrate to a different stemming library.

Thanks again 🙏

Lyra v0.1.0 automation moved this from To do to Done Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed v0.1.0
Projects
No open projects
Development

No branches or pull requests

3 participants