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

v4.3.0 breaks testem #4121

Closed
chriskrycho opened this issue Oct 14, 2021 · 16 comments
Closed

v4.3.0 breaks testem #4121

chriskrycho opened this issue Oct 14, 2021 · 16 comments
Labels
bug Something isn't working
Milestone

Comments

@chriskrycho
Copy link
Contributor

chriskrycho commented Oct 14, 2021

Describe the bug
As of the v4.3.0 release, testem (and therefore all CI runs which use it, so this will shortly be hitting the whole Ember.js community and any others as they upgrade or for runs which check against "drifted dependencies") is failing with the following error:

io.connect is not a function

Inspecting io shows that it is in fact the lookup function (dist).

Screen Shot 2021-10-14 at 16 31 05

To Reproduce

Unfortunately, this is a bit involved, and I don't have a good minimal reproduction yet. I will add one later. For now, I will link directly to the relevant bits of testem's source.

Socket.IO server version: 4.3.0

Server

  • importing socket.io as `const socketIO = require('socket.io');
  • using the imported value as a function

Socket.IO client version: x.y.z

Client

Expected behavior
Given 4.3.0 was a patch release, testem's previous usage should continue to work without issue.

Platform:

  • Device: Mac or Linux
  • OS: Ubuntu latest, macOS 11.6, etc.

Additional context
I strongly suspect (but haven't yet been able to prove) that this is related to either the engine.io bump, serving ESM, or the intersection of the two.

@chriskrycho chriskrycho added the bug Something isn't working label Oct 14, 2021
@chriskrycho
Copy link
Contributor Author

Update: this is actually an issue in socket-io.client; please feel free to move it!

@chriskrycho
Copy link
Contributor Author

The change to the shape of the API was introduced here.

@chriskrycho
Copy link
Contributor Author

A further update: it's not actually clear how or why the build worked previously on the 4.x series. As I'm looking at the shape of the modules exported, it doesn't seem like it should have. I currently suspect that the switch from webpack to Rollup changed the shape of the generated output in a way which happened to break testem; but it looks like the change to the API itself is something testem possibly should have absorbed some time ago.

@chriskrycho
Copy link
Contributor Author

Confirmed: the output from webpack on v4.2.0 included exports.connect = lookup, here, which meant that users could call io.connect previously; the Rollup output does not have a top-level value of connect defined anywhere on the result of the factory() call Rollup generates.

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Oct 14, 2021

Not the webpack→Rollup transition's fault—a little further digging back into the change shows what happened: changing from modules.exports to a standard ES named export (here and here respectively) resulted in different compiled output, because you also switched to doing a single default export here rather than re-exporting all the values as a namespace. For equivalent semantics to what you had previously, the contents of lib/browser-entrypoint.ts should be:

export * as io from './index.js';

I'm happy to open a PR to that effect tomorrow.

chriskrycho added a commit to chriskrycho/socket.io-client that referenced this issue Oct 14, 2021
The previous (CJS module) export semantics were of a *namespace*-style
export, while the semantics introduced in c76d367 are of a single value
exported as the default value instead. This broke downstream consumers
who were using the namespace-style export, as documented at
[socketio/socket.io#4121][4121]. While the single default export should
work fine (and might be preferable!), it's a breaking change, and
appears to be an *accidental* breaking change, so this reverts to the
previous semantics.
gilest added a commit to adopted-ember-addons/ember-file-upload that referenced this issue Oct 15, 2021
Version 4.3.0 of socket.io breaks testem connection when running tests headless.

Reference socketio/socket.io/issues/4121
@darrachequesne
Copy link
Member

Arf, sorry for that.

export * as io from './index.js';

is bundled as:

[...]
  var index = /*#__PURE__*/Object.freeze({
    __proto__: null,
    Manager: Manager,
    Socket: Socket,
    io: lookup,
    connect: lookup,
    protocol: protocol
  });

  exports.io = index;

  Object.defineProperty(exports, '__esModule', { value: true });

Which means the io object would be nested: io.io.connect(...)

The following should work though:

import { io, connect } from "./index.js";

io.connect = connect;

export default io;

What do you think?

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Oct 15, 2021

That could work, but in that case (for the sake of backwards compatibility) we should make sure we do that for all the functions which were previously exported that way. Testem also relies on having io.Manager available, for example. I wonder if it makes sense to make sure that lookup/io has that before exporting in the source module, rather than trying to handle it on the entry point side.

@chriskrycho
Copy link
Contributor Author

Aside: JavaScript is such shenanigans. The fact that an export can be both a function and a namespace is just… 🙃😩🤪🥴

@chriskrycho
Copy link
Contributor Author

I've pushed an update to the PR which manually recreates the namespace in the source module, while preserving the nice new ES module exports.

bertdeblock added a commit to chrislopresto/ember-freestyle that referenced this issue Oct 15, 2021
bertdeblock added a commit to chrislopresto/ember-freestyle that referenced this issue Oct 15, 2021
darrachequesne pushed a commit to socketio/socket.io-client that referenced this issue Oct 15, 2021
This restores the previous behavior, where the "io" object available in
the browser could be used as a function (`io()`) or as a namespace
(`io.connect()`).

The breaking change was introduced in [1].

Related: socketio/socket.io#4121

[1]: 16b6569
@darrachequesne
Copy link
Member

This should be fixed by socketio/socket.io-client@8737d0a, which was included in ̀socket.io-client@4.3.1.

@SergeAstapov
Copy link

@chriskrycho @darrachequesne I maybe missing something, as far as I can see we need a new release of socket.io because client is bundled with socket.io, in the client-dist folder.

E.g. Testem.js does not depend on socket.io-client, it only has socket.io in it's dependencies

@chriskrycho
Copy link
Contributor Author

@SergeAstapov yeah, you're correct—given the re-bundling pattern used socket.io itself also needs a release. @darrachequesne, mind cutting that on Monday? Thanks!

@hoIIer
Copy link

hoIIer commented Oct 17, 2021

@chriskrycho assuming ember-cli will need to cut a release once testem updates as well? (currently broken in latest 3.28)

@chriskrycho
Copy link
Contributor Author

Shouldn't. The CI breakage is just from the Ember CI defaults of including a run which lets all dependencies resolve to their latest legal version (using --no-lockfile), precisely so we catch issues like this early. Once the release is out, all of the CI jobs should start passing again.

If you're seeing the CI builds fail from this issue (as I have on ember-modifier), it's almost certainly because of a cache issue which is causing other runs to inherit the bad version even though per the lock file they shouldn't. That should also get fixed (via the same bad cache behavior!) once socket.io cuts 4.3.1. I'm going to poke at the CI config a bit on Monday and see if there's something we need to tweak for the GH Actions defaults to make sure that part doesn't become a problem again in the future.

@hoIIer
Copy link

hoIIer commented Oct 17, 2021

ok gotcha thanks for the work & update

@darrachequesne
Copy link
Member

@chriskrycho done! https://github.com/socketio/socket.io/releases/tag/4.3.1

@darrachequesne darrachequesne added this to the 4.3.1 milestone Oct 18, 2021
sunrise30 added a commit to sunrise30/socket.io-client that referenced this issue Jan 8, 2022
This restores the previous behavior, where the "io" object available in
the browser could be used as a function (`io()`) or as a namespace
(`io.connect()`).

The breaking change was introduced in [1].

Related: socketio/socket.io#4121

[1]: socketio/socket.io-client@16b6569
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants