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

update wrapper.mjs so it provides the same default as for CommonJS users #4100

Closed
wants to merge 2 commits into from

Conversation

trusktr
Copy link

@trusktr trusktr commented Sep 17, 2021

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 behavior

When migrating from CommonJS to ESM, the end user no longer had the default option available unless they import io from 'socket.io/dist/index.js'.

You can see at the bottom of this file: https://unpkg.com/socket.io@4.2.0/dist/index.js

There is this line:

module.exports = (srv, opts) => new Server(srv, opts);

which is a convenience for CommonJS users, but ESM users don't have this option (unless they import dist/index.js)

New behavior

Now they can import io from 'socket.io' which is similar to const io = require('socket.io') as they had when using CommonJS.

Other information (e.g. related issues)

n/a

…with npm works

The downside of this is that yarn doesn't follow npm spec, and yarn needs prepack instead of prepare, so install from git with yarn will no longer work. Either npm works, or yarn works. I think it's a better choice to support the standard tool for npmjs.com.
@@ -42,7 +42,7 @@
"test:unit": "nyc mocha --require ts-node/register --reporter spec --slow 200 --bail --timeout 10000 test/socket.io.ts",
"format:check": "prettier --check \"lib/**/*.ts\" \"test/**/*.ts\"",
"format:fix": "prettier --write \"lib/**/*.ts\" \"test/**/*.ts\"",
"prepack": "npm run compile"
"prepare": "npm run compile"
Copy link
Author

Choose a reason for hiding this comment

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

use prepare instead of prepack so that installing socket.io from git with npm works

The downside of this is that yarn doesn't follow npm spec, and yarn needs prepack instead of prepare, so install from git with yarn will no longer work. Either npm works, or yarn works. I think it's a better choice to support the standard tool for npmjs.com.

@trusktr
Copy link
Author

trusktr commented Sep 17, 2021

Note, the types need an update. I'm in a plain JS project, so no issue for me, but TS users will get a type error:

Screenshot from 2021-09-16 23-10-37

Because of this, I'll mark this PR as a draft.

@trusktr trusktr marked this pull request as draft September 17, 2021 06:11
@trusktr trusktr changed the title update wrapper.mjs so it provides the same default as for CommonJS users [DRAFT] update wrapper.mjs so it provides the same default as for CommonJS users Sep 17, 2021
@trusktr trusktr changed the title [DRAFT] update wrapper.mjs so it provides the same default as for CommonJS users update wrapper.mjs so it provides the same default as for CommonJS users Sep 17, 2021
@trusktr
Copy link
Author

trusktr commented Sep 17, 2021

I was wrong above, actually if a user tries to import socket.io/dist/index.js this will get an error like this:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/index.js' is not defined by "exports" in /home/trusktr/project/node_modules/socket.io/package.json imported from /home/trusktr/project/src/some-file.js

So that option is not even on the table, unless they perform an import using a relative path that includes the node_modules folder like this:

import io from '../node_modules/socket.io/dist/index.js'

But as you know, this is less reliable because the structure inside node_modules is not guaranteed.

Another workaround is the user can easily define this function too, though of course it would be better if it existed symmetrically for both CJS or ESM users:

import {Server} from 'socket.io'

const io = (srv, opts) => new Server(srv, opts);
export default io

This has the downside that if socket.io changes the implementation of the default CJS io export, the end user has to update it as well.

@darrachequesne
Copy link
Member

Hi! This is deliberate. ESM users should use the named export:

import { Server } from "socket.io";

Reference: https://socket.io/docs/v4/server-initialization/

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.

2 participants