-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Stop using CommonJS or AMD dependencies #5118
Comments
Actually, we provide a debug-free ESM build: https://github.com/socketio/socket.io-client/blob/9b235ec01d2ea7f1685b704bd2c5001597635f51/package.json#L26-L30 Not sure why |
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: 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). |
OK, I think I found the culprit: due to socketio/socket.io-client@781d753 (included in v4.7.0), the build that includes the 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 |
I think, if you have to include that |
Totally agree 👍
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: Reference: https://nodejs.org/api/packages.html#community-conditions-definitions
@FossPrime do you have any thoughts on this? |
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:
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. 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. |
This reverts the previous commit ([1]), and expose the ESM build that includes the debug package on a subpath: ```js import { Socket } from "engine.io-client/debug"; ``` Reference: https://nodejs.org/api/packages.html#subpath-exports Related: https://github.com/socketio/socket.io-client/issues/1586 [1]: fe4d93a
This reverts the previous commit ([1]), and expose the ESM build that includes the debug package on a subpath: ```js import { Socket } from "socket.io-client/debug"; ``` Reference: https://nodejs.org/api/packages.html#subpath-exports Related: https://github.com/socketio/socket.io-client/issues/1586 [1]: 781d753
This reverts the previous commit ([1]), and expose the ESM build that includes the debug package on a subpath: ```js import { io } from "socket.io-client/debug"; ``` Reference: https://nodejs.org/api/packages.html#subpath-exports Related: https://github.com/socketio/socket.io-client/issues/1586 [1]: 781d753
I have tested v4.7.1, and it seems ok. Well done! Closing it now ;) |
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:
|
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 |
I considered this, and it sounds good, but the compatibility of this solution is not universal.
Why not use 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. |
See also this: https://twitter.com/deno_land/status/1643715218793701381?s=20 |
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/... |
True, but having people open issues here about the webpack warning was not really thrilling either.
That would be ideal indeed. There is also this fork of Also: vercel/ms#184 |
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:
And if we follow that link...
So basically, we can use that flag to suppress the warnings:
But hiding the issue isn't really a solution. A library that targets specifically web should NOT include such old CommonJS dependencies anymore ;)
The text was updated successfully, but these errors were encountered: