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

The update to ws 1.0.x introduces breaking changes #87

Closed
bengillies opened this issue Jan 5, 2016 · 7 comments
Closed

The update to ws 1.0.x introduces breaking changes #87

bengillies opened this issue Jan 5, 2016 · 7 comments
Assignees
Labels
bug
Milestone

Comments

@bengillies
Copy link

@bengillies bengillies commented Jan 5, 2016

Upgrading from ws 0.8.x to 1.0.x introduces several breaking changes in the ws library which appear to be incompatible with other modules (for me notably karma breaks as it's still on ws 0.8.x via socket.io).

Not sure exactly what's causing the breaking, but nes 2.0.1 should probably have been released as 3.0.0 at least.

For those interested, running Karma (with webpack) seems to complain about:

CriticalDependenciesWarning: Critical dependencies:
76:22-40 the request of a dependency is an expression
76:43-53 the request of a dependency is an expression
ModuleParseError: Module parse failed: ~/src/wave/ui/node_modules/bindings/README.md Line 2: Unexpected token ===
You may need an appropriate loader to handle this file type.
| node-bindings
| =============
| ### Helper module for loading your native module's .node file
|
ModuleParseError: Module parse failed: 
~/src/wave/ui/node_modules/bindings/package.json Line 2: Unexpected token :
You may need an appropriate loader to handle this file type.
| {
|   "_args": [
|     [
|       "bindings@1.2.x",

Running the same thing with nes 2.0.0 seems to work fine.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jan 5, 2016

@kpdecker can you help figure this out?

I am worried about publishing this as a breaking change since 2.0.0 includes a critical security bug.

@kpdecker

This comment has been minimized.

Copy link
Contributor

@kpdecker kpdecker commented Jan 5, 2016

Regarding the webpack error, @bengillies can you try adding this to your webpack config?

        {
          test: /\.json$/,
          loader: require.resolve('json-loader')
        },

You may need to npm install json-loader.

Can't comment on the 0.8 vs. 1.0 changes in ws as I'm not familiar with what changes occurred in the lib.

@kpdecker

This comment has been minimized.

Copy link
Contributor

@kpdecker kpdecker commented Jan 5, 2016

I missed that there were two errors in there. The first looks like there is some sort of require('README.md') going on, which seems quite weird to me, but I suspect that this is related to the "the request of a dependency is an expression" message. @bengillies is there more to these logs? Is this on a public project or could you otherwise post a simplified project that demonstrates the build issue?

@bengillies

This comment has been minimized.

Copy link
Author

@bengillies bengillies commented Jan 6, 2016

So I've looked a bit closer and afaict it seems like webpack is getting confused between different versions of ws.

I can fix the issue by changing the following in lib/client.js:

-    const WS = /* $lab:coverage:off$ */ (typeof WebSocket === 'undefined' ? require('ws') : WebSocket); /* $lab:coverage:on$ */       // Using require vs proper UMD binding as we assume WebSocket is available through native bindings in all environments
+    const WS = /* $lab:coverage:off$ */ WebSocket; /* $lab:coverage:on$ */

Given the below change in ws 1.0.0, I'm not sure what's gained by keeping the require('ws') in the client codebase, or what the alternative would be, but it seems like webpack isn't clever enough to cope with both versions and the conditional require.

Removed the client code. It was simple wrapper that really doesn't belong in a full ledged node.js library. If you want browser support you could just conditionally import it the WebSocket server. var WS = window.WebSocket || require('ws')

@tkh44

This comment has been minimized.

Copy link
Contributor

@tkh44 tkh44 commented Jan 6, 2016

We fixed this by mocking fs and tls in our webpack config.
http://webpack.github.io/docs/configuration.html#node

entry: 'lib/index.js',
node: {
    fs: 'empty',
    tls: 'empty'
}
/* other webpack settings */

This works for now, but we need to figure out a proper way to conditionally require 'ws'

@hueniverse hueniverse closed this in b2c135b Jan 6, 2016
@hueniverse hueniverse added the bug label Jan 6, 2016
@hueniverse hueniverse added this to the 2.0.2 milestone Jan 6, 2016
@hueniverse hueniverse self-assigned this Jan 6, 2016
@kpdecker

This comment has been minimized.

Copy link
Contributor

@kpdecker kpdecker commented Jan 6, 2016

Academic with the other fix, but it appears that the issues above is due to this line (I'm guessing without a repo case to test against) but I'm not clear how exactly that relates to ws since a vanilla install of ws does not introduce this module nor do I see any obvious references to this in the regular suspects in the webpack stack.

@bengillies

This comment has been minimized.

Copy link
Author

@bengillies bengillies commented Jan 7, 2016

I'm not clear how exactly that relates to ws

I found that too. iirc, it's used by bufferutil and utf-8-validate, which are required by the old version of ws (used by karma in my case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.