search

socketio / socket.io

Realtime application framework (Node.JS server)
https://socket.io
MIT License
60.88k stars 10.09k forks source link

Stop using CommonJS or AMD dependencies #5118

Open vitaly-t opened 1 year ago

vitaly-t commented 1 year ago

This library uses debug dependency, which prevents any code optimization.

For example, in an Angular web client, we get the following warning in the browser's console:

image

And if we follow that link...

So basically, we can use that flag to suppress the warnings:

image

But hiding the issue isn't really a solution. A library that targets specifically web should NOT include such old CommonJS dependencies anymore ;)

darrachequesne commented 1 year ago

Actually, we provide a debug-free ESM build: https://github.com/socketio/socket.io-client/blob/9b235ec01d2ea7f1685b704bd2c5001597635f51/package.json#L26-L30

Not sure why ./build/esm-debug/index.js is resolved in your case, instead of ./build/esm/index.js.

vitaly-t commented 1 year ago

I'm not really sure what is going on, but it should be very easy to reproduce - just generate a new Angular v16.x project and add this library as a dependency.

b.t.w. If I try to change the import into a specific folder like this:

import {Manager, Socket} from 'socket.io-client/build/esm';

it does not build, comes out with error:

image

I guess it is not supposed to be imported that way. But the default import results in that warning in the browser console (unless explicitly suppressed in the project's configuration).

darrachequesne commented 1 year ago

OK, I think I found the culprit: due to https://github.com/socketio/socket.io-client/commit/781d753a626d01e675056a2ff4e27f5dd599564f (included in v4.7.0), the build that includes the debug package (./build/esm-debug/index.js) is now imported during development.

Which is OK I guess, since that allows to print the debug logs to the console, and it won't be included in the final bundle (which will use ./build/esm/index.js). @vitaly-t thoughts?

vitaly-t commented 1 year ago

I think, if you have to include that debug package, then it should not be the default, or it will confuse developers. I would either expose it through an option, or an explicit import, like socket.io-client/debug, just not the default import, because it breaks ESM tree-shaking and causes warnings.

darrachequesne commented 1 year ago

or it will confuse developers.

Totally agree :+1:

just not the default import

Just to be sure to be on the same page, it's not really the default import, it's the import when the "development" condition is met:

https://github.com/socketio/socket.io-client/blob/9b235ec01d2ea7f1685b704bd2c5001597635f51/package.json#L26-L30

Reference: https://nodejs.org/api/packages.html#community-conditions-definitions

import { io } from "socket.io-client/debug" sounds reasonable.

@FossPrime do you have any thoughts on this?

FossPrime commented 1 year ago 
import {Manager, Socket} from 'socket.io-client/build/esm';

I guess it is not supposed to be imported that way

Socket.io specifies subpath exports. Which with modern module resolution, prevents access to undeclared endpoints. That same module resolution is causing dev-mode webpack to load the debug package. Tree shacking or production builds should not be affected.

This warning should also be present in Vite's strict modes.

Potential solutions to remove the dev warning:

  1. Provide direct access to the builds with a build/**/* export, allowing people to override module resolution. I've seen other projects do this successfully.
  2. Use another debug package that's ESM friendly, I have not found a good one... So I usually end up writing a mini version of it.
  3. PR a browser friendly Debug update. Should be much easier than trying to do the whole library like I did (terminal colors, inheritance and module resolution acrobatics make it hard). https://github.com/debug-js/debug/issues/786

If your bundler is asking for a development version with good debuggability and source maps, we should provide it what it's asking for.

Feathers removed the debug dependency for Deno support, and it's caused far worse DX. This is a growing, common, upstream problem, that should ideally have a common solution. The people who maintain Debug are active and receptive... But no-one has the endurance to try making a fully backward compatible ESM update.

We should definitely do 1. 3 would be ideal. But 2 is probably more practical in the short term.

If you publish a debug package under @socketio/debug I'll gladly contribute to it and promote it.

vitaly-t commented 1 year ago

I have tested v4.7.1, and it seems ok. Well done! Closing it now ;)

FossPrime commented 1 year ago

This is worse, for everyone... As now we have to manually turn on and off the debug build and run npm i... Rather than the clean automatic operation we had before.

Even in webpack the biggest downside was a small easy to filter warning in dev.

Using the debug build also doesn't turn on debug as you might expect... You still have to configure debug. Which is a bit unexpected when you've already explicitly asked for debug. This is unexpected behavior at the worst time.

The current situation is slightly better than 4.6.x, but worse than 4.7.0. as debug was the primary way to see messages in some environments.

The problem wasn't that the development export was debugable but that the debug logger we and 250M people a week use, has some well known drawbacks.

With 4.7.1 downstream libraries that bundle socketio internally have to ALSO make a /debug export for this to work, with some custom bundling that swaps /debug imports This is the exact problem the debug package and subpath exports were intended to fix.

Proposed solution:

  1. Implement an ESM debug logger and return the development build.
vitaly-t commented 1 year ago

In this case, maybe a better solution would have been to make use of dynamic import, and an explicit configuration parameter that activates debugging?

Or, perhaps even better solution - move debug into peerDependencies, and have this library check at run-time for presence, and only then use it. I think it's the cleanest approach.

FossPrime commented 1 year ago

move debug into peerDependencies, and have this library check at run-time for presence, and only then use it

I considered this, and it sounds good, but the compatibility of this solution is not universal.

  1. Deno has no dynamic import support.
  2. Yarn 2 and PNPM Plug and Play may bundle debug 100% of the time, as to my knowledge they isolate all modules with a deterministic dependency set, causing the same warning in the webpack-dev server.

Why not use https://github.com/kat-tax/vslite/blob/1de9b5d678719e6b18151fd6409a32769939e3a9/src/utils/debug.ts With picomatch added, the behavior will be 99% identical to debug, but ESM friendly.

We could even expose that debug logger under the /debug export, and encourage others to use it as a debug package alternative. Tree shaking means it would be nearly free for people doing that.

vitaly-t commented 1 year ago

Deno has no dynamic import support.

Are you sure? :)

See also this:

https://twitter.com/deno_land/status/1643715218793701381?s=20

FossPrime commented 1 year ago

Are you sure? :)

Might be a new or not default feature... I've never gotten it to work on Deno Playground https://deno.com/deploy/docs/playgrounds

Someone should just do a hard ESM wrap around debug, setup a Cron job for updates and call it a day.

Or better yet, start an NPM organization dedicated to this task... like DefinitelyTyped did with @types/...

darrachequesne commented 1 year ago

With 4.7.1 downstream libraries that bundle socketio internally have to ALSO make a /debug export for this to work

True, but having people open issues here about the webpack warning was not really thrilling either.

  1. PR a browser friendly Debug update

That would be ideal indeed.

There is also this fork of debug: https://github.com/milahu/debug-esm

Also: https://github.com/vercel/ms/issues/184