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

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

Conversation

dasilvacontin
Copy link

@dasilvacontin dasilvacontin commented May 24, 2020

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

Socket.io server ignores 'socket.io-client' module installed inside socket.io when there's an installation of 'socket.io-client' sibling to 'socket.io'.

New behaviour

Now uses closest 'socket.io-client' installation.

Other information (e.g. related issues)

Fix is by @Apollon77, taken from PR #3557. Their fix fixes #3555.

I added a failing test to show it actually had a behavior issue prior to the fix, and give an example of how it fails. Plus to prevent a regression in the future.

@dasilvacontin dasilvacontin force-pushed the add-failing-test-plus-fix-for-3555 branch from a05b686 to 5492655 Compare May 24, 2020 16:44
Apollon77 and others added 2 commits May 24, 2020 17:47
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!
Co-authored-by: David da Silva <[email protected]>
@dasilvacontin dasilvacontin changed the title Add failing test for #3555 Add failing test for #3555 + the fix itself May 24, 2020
@darrachequesne
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

Served socket.io client does not find client installed in own node_modules
3 participants