Skip to content

Commit

Permalink
Use built version os sockjs-client (#1148)
Browse files Browse the repository at this point in the history
`webpack-dev-server` bundles `sockjs-client` from source. `sockjs-client` sources assume a node environment however, and will fail to run if `global` is not available.

By bundling `sockjs-client` from source, the node environment requirement from `sockjs-client` will also be required of the app being served and thus it is not possible to have `node.global = false` in the user app being bundled.

This fix uses the built `sockjs-client` instead of the sources.

Fix #1147
  • Loading branch information
filipesilva authored and shellscape committed Oct 16, 2017
1 parent 32412bb commit 06df2f4
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 2 deletions.
2 changes: 1 addition & 1 deletion client/socket.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const SockJS = require('sockjs-client');
const SockJS = require('sockjs-client/dist/sockjs');

let retries = 0;
let sock = null;
Expand Down
9 changes: 9 additions & 0 deletions examples/node-false/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# CLI - node false

```shell
node ../../bin/webpack-dev-server.js --open
```

## What should happen

In the app you should see "It's working."
3 changes: 3 additions & 0 deletions examples/node-false/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';

document.write("It's working.");
9 changes: 9 additions & 0 deletions examples/node-false/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<script src="bundle.js" type="text/javascript" charset="utf-8"></script>
</head>
<body>
<h1>Example: CLI - node false</h1>
</body>
</html>
7 changes: 7 additions & 0 deletions examples/node-false/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

module.exports = {
context: __dirname,
entry: './app.js',
node: false
};
3 changes: 2 additions & 1 deletion test/fixtures/simple-config/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ module.exports = {
output: {
filename: 'bundle.js',
path: '/'
}
},
node: false
};

12 comments on commit 06df2f4

@slate71
Copy link

@slate71 slate71 commented on 06df2f4 Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filipesilva This change (line 3 in client/socket.js) prevents my app from building as there's no dist directory in the published socketjs-client package. The directory is ignored for npm. Am I missing something?

https://github.com/sockjs/sockjs-client/blob/master/.npmignore

@michael-ciniawsky
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!dist/sockjs.js
!dist/sockjs.js.map
!dist/sockjs.min.js
!dist/sockjs.min.js.map
=> ! <= dist/socksjs.js

But not 💯 if that works like I assume. Could you clean the npm cache, delete the lockfile and install afresh ?

@filipesilva
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slate71 what version of sockjs-client are you using? 1.1.4 seems to have it published: https://unpkg.com/sockjs-client@1.1.4/dist/sockjs.js

@slate71
Copy link

@slate71 slate71 commented on 06df2f4 Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, that is correct, v1.1.4 does have a dist directory. Yet, after cleaning the cache and installing afresh (and confirming the dist directory is there with the built files) I get this error.

ERROR in (webpack)-dev-server/client/socket.js
Module not found: Error: Can't resolve 'sockjs-client/dist/sockjs'

@shellscape
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slate71 what's the dir structure of node_modules/sockjs-client look like?

@slate71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@slate71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is there, using a relative path const SockJS = require('../../sockjs-client/dist/sockjs');, will find the module. Don't yet understand why const SockJS = require('sockjs-client/dist/sockjs'); will not.

I think the issue is on my side, though, and not with this commit.

@shellscape
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that looks like windows cmd output. here's hoping it's not a platform issue.

@slate71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, no I'm on a mac.

@filipesilva
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on windows myself, and I haven't observed odd cross platform behaviour (on this).

I wonder if it is related to the webpack configuration though? The resolve entries. Although that shouldn't have worked before either then... Unless you have several versions of sockjs, and only the topmost is being found?

@slate71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out. It was indeed on our side. We had an alias in our webpack config for sockjs-client I was unaware of and didn't think to check for.

  'sockjs-client': 'sockjs-client/lib/entry'

No dist directory there.

@filipesilva
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why that was there. Perhaps for a similar reason as this PR?

Please sign in to comment.