Make sure correct client files are served, fixes #3555 #3557

Apollon77 · 2020-02-23T13:33:12Z

The old logic missed some places and was not able to look into the node_modules of the socket.io library itself.

The new logic always uses require.resolve to lookup the "correct" location for the socker.io-client library and then construct the path correctly

This fixes #3555

A fast release of a fixed version would be awesome!

The kind of change this PR does introduce

Current behaviour

New behaviour

Other information (e.g. related issues)

The old logic missed some places and was not able to look into the node_modules of the socket.io library itself. The new logic always uses require.resolve to lookup the "correct" location for the socker.io-client library and then construct the path correctly This fixes socketio#3555 A fast release of a fixed version would be awesome!

dasilvacontin

Relevant docs:

I think the comment inside the function should explain the logic and concerns (e.g., the concerns that cause the #3555 bug).

And ideally we have a regression test as well.

lib/index.js

dasilvacontin · 2020-05-03T16:59:01Z

lib/index.js

-    }
-    return require.resolve(file);
+    var clientPath = require.resolve('socket.io-client'); // resolve the client dep. end in lib/index
+    return path.normalize(path.join(path.dirname(clientPath), '..', '..', file));


For the sake of code legibility, I'd be great to have each function execution on a separate line, assigning the result to a variable with a readable+understandable variable name.

If needed I can do that, but is it not undrstandable?
Else my proposals would be

var clientLibDir = path.dirname(clientPath) var clientFilePath = path.join(clientLibDir, '..', '..', file);

(Just to be clear, I don't think the original code is very legible either).

My intuition is something like:

var pathToClosestSocketIOClientMainFile = require.resolve('socket.io-client'); var pathToParentNodeModulesFolder = path.join(path.dirname(clientPath), '..', '..') return path.normalize(path.join(pathToParentNodeModulesFolder, file));

Imo, ideally code is readable without comments.

"If needed I can do that, but is it not undrstandable?"

We have the context, but someone else might not know why not just do require.resolve(file). (Which I just realized isn't really addressed in my suggestion, and it should. Maybe a comment)

And 2), to someone just reading, path.join(clientLibDir, '..', '..', file) is quite confusing. Like, where are you trying to go?? Why .. .. ? etc

Now you know how long I needed to understand the problem ;-) But yes you were right ... so i "learned" the context the hard way.

I think your names (even veeery long) are ok and basically understandable. Maybe some more details in the JSDoc of the method would so help to understand what is done there. Then maybe good variable names are enough.

Co-authored-by: David da Silva <[email protected]>

Apollon77 · 2020-05-04T05:28:28Z

And ideally we have a regression test as well.

What you mean by that?

dasilvacontin · 2020-05-06T14:44:40Z

@Apollon77 A regression test is a type of test that makes sure that a previously known bug is no longer happening in the code.

Here's socket.io's test folder: https://github.com/socketio/socket.io/tree/master/test

Here's how the tests are run: https://github.com/socketio/socket.io/blob/master/package.json#L24

Ideally we have that regression test to 1) make sure that the bug is real, 2) make sure that the fix actually works (and doesn't make something else stop working (hopefully you have tests for other cases/functionality)), and 3) make sure that in the future someone doesn't make a change to the code that makes the bug happen again.

If you don't happen to be interested in coding it, happy to take care of it! :)

Apollon77 · 2020-05-06T21:07:15Z

If you don't happen to be interested in coding it, happy to take care of it! :)

In general I would love to do it but it would take me some weeks to find time for it. We currently prepare a big release in one of my main projects and I'm spending all my free time to complete and manage this at the moment.
So if you have time before me I would be happy to review your tests.

As soon as it is accepted and released I can also remove my workarounds from some projects ;-)

Apollon77 · 2020-05-06T21:13:36Z

BTW: This is the current workaround I found to work around that issue :-)

https://github.com/ioBroker/ioBroker.socketio/blob/master/lib/socket.js#L1669-L1705

dasilvacontin · 2020-05-07T10:01:03Z

BTW: This is the current workaround I found to work around that issue :-)

https://github.com/ioBroker/ioBroker.socketio/blob/master/lib/socket.js#L1669-L1705

Cool, this is great insight, thanks! :)

dasilvacontin · 2020-05-07T10:13:55Z

Okay, I've been testing a bit more to try to understand the problem, and it feels like the problem you are running into is that, if your socket.io installation has a node_modules inside it (with an installation of socket.io-client), it is ignored when trying to seek an installation of socket.io-client, defaulting to checking first the node_modules folder that contains the socket.io installation. Is this spot on, @Apollon77?

If this is the case, to describe the bug/need better in the regression test / documentation, what's making you have a node_modules folder inside your socket.io installation?

Thank you @Apollon77! :)

Apollon77 · 2020-05-08T16:53:34Z

The reason for it is "npm" and how they organize the packages. The topic here is that we have a system which is extenable by plugins. In fact they all end up in one big "node_modules" structure.

And the issue here was that some package installed one version of socket.io (1.7.x) and because it was the first one, npm put on top level as usual.

Now a second package came and wanted to have socket.io 2.3.0 which is by "usual carrot deps" incompatible. So npm is not allowed to overwrite the "socket.io placed in root node-modules" but put it in the node-modules in the local package.

This is a complete normal logic on how npm strcutures the package files.

Node.js is resolving with the following logic: get the directory of the js-file that requires a module and check if there is a node-modules and check if required module is there. If not, go one directory to up, check the same there ... end if you found the module or you end up in "root" :-)

The resolve logic from socket.io in 2.3.0 ignored the possibility that there could be an "own"/local node-modules and starts one level too high to search for the package ... and so we ended in always finding the socket.io-client 1.7.x package which was wrong in this case.

Better understandable?

dasilvacontin · 2020-05-09T20:20:44Z

@Apollon77 That's perfect, thank you!! :)

Might be a bit busy the upcoming week though (doing a design sprint) but will come back to this around May 19.

Apollon77 · 2020-05-09T23:19:35Z

np ... my workaroud works fine so far, so no need to hurry (at least not foru me) :-)

dasilvacontin · 2020-05-24T16:50:51Z

Added a failing test in #3604. You can see that it failed previous to pulling in your fix, and how it passes after pulling in your fix: https://github.com/socketio/socket.io/pull/3604/commits.

Apollon77 · 2020-05-24T18:08:30Z

great work @dasilvacontin

darrachequesne · 2021-01-14T23:31:58Z

Superseded by 7603da7 (included in [email protected]). Thanks for the PR!

vusolapohvistr mentioned this pull request Mar 10, 2020

Socket.io sends broken stream #3563

Closed

2 tasks

dasilvacontin suggested changes May 3, 2020

View reviewed changes

Update lib/index.js

e60517f

Co-authored-by: David da Silva <[email protected]>

dasilvacontin mentioned this pull request May 24, 2020

Add failing test for #3555 + the fix itself #3604

Closed

5 tasks

darrachequesne closed this Jan 14, 2021

darrachequesne added this to the 3.0.0 milestone Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure correct client files are served, fixes #3555 #3557

Make sure correct client files are served, fixes #3555 #3557

Apollon77 commented Feb 23, 2020

dasilvacontin left a comment

dasilvacontin May 3, 2020

Apollon77 May 4, 2020

dasilvacontin May 6, 2020

dasilvacontin May 6, 2020 •

edited

Loading

Apollon77 May 6, 2020

Apollon77 commented May 4, 2020

dasilvacontin commented May 6, 2020 •

edited

Loading

Apollon77 commented May 6, 2020

Apollon77 commented May 6, 2020

dasilvacontin commented May 7, 2020

dasilvacontin commented May 7, 2020 •

edited

Loading

Apollon77 commented May 8, 2020 •

edited

Loading

dasilvacontin commented May 9, 2020

Apollon77 commented May 9, 2020

dasilvacontin commented May 24, 2020

Apollon77 commented May 24, 2020

darrachequesne commented Jan 14, 2021

Make sure correct client files are served, fixes #3555 #3557

Make sure correct client files are served, fixes #3555 #3557

Conversation

Apollon77 commented Feb 23, 2020

The kind of change this PR does introduce

Current behaviour

New behaviour

Other information (e.g. related issues)

dasilvacontin left a comment

Choose a reason for hiding this comment

dasilvacontin May 3, 2020

Choose a reason for hiding this comment

Apollon77 May 4, 2020

Choose a reason for hiding this comment

dasilvacontin May 6, 2020

Choose a reason for hiding this comment

dasilvacontin May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Apollon77 May 6, 2020

Choose a reason for hiding this comment

Apollon77 commented May 4, 2020

dasilvacontin commented May 6, 2020 • edited Loading

Apollon77 commented May 6, 2020

Apollon77 commented May 6, 2020

dasilvacontin commented May 7, 2020

dasilvacontin commented May 7, 2020 • edited Loading

Apollon77 commented May 8, 2020 • edited Loading

dasilvacontin commented May 9, 2020

Apollon77 commented May 9, 2020

dasilvacontin commented May 24, 2020

Apollon77 commented May 24, 2020

darrachequesne commented Jan 14, 2021

dasilvacontin May 6, 2020 •

edited

Loading

dasilvacontin commented May 6, 2020 •

edited

Loading

dasilvacontin commented May 7, 2020 •

edited

Loading

Apollon77 commented May 8, 2020 •

edited

Loading