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

Problem of usage in nodejs 14.5 with the lastest version of @deepstream/client@5.1.2 #534

Closed
geohuz opened this issue Jul 11, 2020 · 10 comments

Comments

@geohuz
Copy link

geohuz commented Jul 11, 2020

I got the following error:


import {DeepstreamClient} from '@deepstream/client'
        ^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@deepstream/client' is expected to be of type CommonJS, 
which does not support named exports. CommonJS modules can be imported by importing 
the default export.
For example:
import pkg from '@deepstream/client';
const {DeepstreamClient} = pkg;

So I changed the usage as below:

import pkgDps from '@deepstream/client'

const {DeepstreamClient} = pkgDps;

But then I get another error:

Error: Cannot find module 'ws'
....
  at ModuleJob.run (internal/modules/esm/module_job.js:140:23)
    at async Loader.import (internal/modules/esm/loader.js:162:24) {
  code: 'MODULE_NOT_FOUND'
}

I also found @deepstream/client@5.1.1 doesn't have this issue

@geohuz geohuz changed the title Problem of usage in nodejs 14.5 Problem of usage in nodejs 14.5 with the lastest version of @deepstream/client@5.1.1 Jul 11, 2020
@yasserf
Copy link
Contributor

yasserf commented Jul 12, 2020

@jaime-ez also related to the last release 😅

@geohuz geohuz changed the title Problem of usage in nodejs 14.5 with the lastest version of @deepstream/client@5.1.1 Problem of usage in nodejs 14.5 with the lastest version of @deepstream/client@5.1.2 Jul 12, 2020
@jaime-ez
Copy link
Collaborator

jaime-ez commented Jul 12, 2020

Ok, I can confirm the issue, it's present with node>12 and it's not so simple to solve.

Apparently there are problems when bundling the 'ws' package with webpack, found multiple issues in other repos with no clear solution nor explanation for the problem (websockets/ws#1126, websockets/ws#1538, websockets/ws#1220, webpack/webpack#1019). At least for me, I wasn't able to make it work properly. When I managed to get 'ws' bundled, I got other issues and the bundle size went from ~190kb to ~750kb

I propose 2 options:

1

  • Since we want to provide one package for both browser/react-native and node users, and including 'ws' in the bundle causes a huge penalty for the bundle size for browser/react-native users, implement a postinstall script that will change the main file in package.json depending on a env variable passed along when installing the @deepstream/client package.
  • I assume most of the users are using @deepstream/client in node, so we could default the package.json main to dist/src/deepstream.js
  • If the browser/react-native is the target platform, the package should be installed as: DEEPSTREAM_ENV=browser npm install @deepstream/client and include a postinstall script that will check for this env variable and change the main file in package.json accordingly.

2

  • Just change back the package.json main to dist/src/deepstream.js and give instructions on the readme and docs that for react-native and browser env the main file should be changed manually.

I prefer option 1...Let me know what you think and of course if there is another way to do it.

@timarandras
Copy link

I got this too: Error: Cannot find module 'ws'. Now I see why.

I prefer Option 1, with the remark, that main pointing to dist/src/deepstream.js would work in both node and browser environments out of the box but with the size penalty in browser env you already mentioned.

I don't get it how the postinstall script in browser env will solve the 'ws can't be bundled with webpack' issue. What would the postinstall script do in browser env if it can't bundle ws? That means the ds.min.js minified/bundled file will never be correct due to the ws issue.

Other remarks:
I seem to recall that @deepstream/client worked well with node v13, not sure this is related, nor I remember well.

@jaime-ez
Copy link
Collaborator

I prefer Option 1, with the remark, that main pointing to dist/src/deepstream.js would work in both node and browser environments out of the box but with the size penalty in browser env you already mentioned.

Yes it does work in node and browser, but not in react-native, that's why we changed the main file to the minified bundle in order to make it work in react native (and browser but didn't check at the moment that it will brake the node platform..my mistake).

In summary pointing the main file in package.json to:

  • dist/bundle/ds.min.js: works in browser and react-native (we have Websockets available in both platforms), not in node (we need the ws module)
  • dist/src/deepstream.js: works in node (we use the ws module) and browser (Websockets available), not in react-native (we need the bundled Buffer module, not available in the platform)

The postinstall script will not solve anything related to webpack, it will point the main file to the bundle or not depending on the use case. And as you mention, since for the web it will also work just using the dist/src/deepstream.js file we could just add the env variable for react native (the name is just descriptive of course) DEEPSTREAM_ENV=react-native npm install @deepstream/client and then the post install script will change the main file to the bundle.
This will make the "special" install a narrower use case thus not adding noise for the most common node and browser scenarios.

Comments?

@timarandras
Copy link

timarandras commented Jul 13, 2020

Its clear now.

I saw in websockets/ws#1126 that they work around the bundling using

// in webpack.config.js
externals: {
      bufferutil: "bufferutil",
      "utf-8-validate": "utf-8-validate",
    }

If we put this also into our webpack.prod.js wouldn't it solve the problem? (I assume you've already tried that.)

@jaime-ez
Copy link
Collaborator

Yes I did, It did bundle the package but other problems emerged. Requires more work.
And as we discussed, making the special install for react-native only won't change the bundle size nor add more dependencies.

@yasserf any comments?

@valentinvichnal
Copy link

I prefer the first option, we could also fork this and make a new package for browser and react-native, in the future there might be more differences.

@yasserf
Copy link
Contributor

yasserf commented Jul 14, 2020

Honestly don't mind whatever solution you think is best, as long as it doesn't break current support. Bundling has never been my thing 🙈

@jaime-ez
Copy link
Collaborator

All right. Pushed a new version v5.1.3, that points the main file to dist/src/deepstream.js and has a postinstall script that check for the above mentioned flag for ease of install in react-native. I'll close for now, please re-open if problem persists!

@jaime-ez
Copy link
Collaborator

One last comment regarding bundled files and react-native: #544

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

No branches or pull requests

5 participants