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

MJS build has different shape than non-MJS build #1513

Closed
mattbasta opened this issue Nov 30, 2020 · 7 comments
Closed

MJS build has different shape than non-MJS build #1513

mattbasta opened this issue Nov 30, 2020 · 7 comments

Comments

@mattbasta
Copy link

I'm using Typescript for my project with Webpack 5 to compile.

Typescript uses the definitions from @types/koa, which currently represent the non-MJS build, which has the equivalent of module.exports = class Koa {...}. This is represented in the TS declaration as the equivalent of export = Koa.

The MJS build, however, exports Koa as default. The TS definition for this would be export default Koa.

When importing Koa with import * as Koa from 'koa'; (which is the only way allowed without esModuleInterop enabled), Typescript believes that Koa is the constructor. However, the constructor is actually Koa.default because the import statement switches to the MJS build.

I currently work around this by doing the following in my code:

import * as Koa from 'koa';

function resolve<T>(module: T): T {
  let x = module as any;
  if (x.default) {
    x = x.default;
  }
  return x;
}

const app = new (resolve(Koa))();

Obviously this isn't great, but short of disabling MJS support, it's the best I can do for now.

The underlying issue here is that the MJS support introduced by #1474 was a backwards-incompatible change for folks using an import-aware bundler like Webpack. The mismatch between the type definitions (which are explicit that import Koa from 'koa'; will not work) and the MJS export (where only the default export is set) causes a problem which can only be detected at runtime or in tests.

The fix which seems most apparent to me is two parts:

  1. Add a default export to the non-MJS build
  2. Update the type definitions to remove the export = export and force folks over to the default export, ensuring that the typechecker will catch these issues immediately rather than at CI/runtime.
@stephenmathieson
Copy link

This sounds like an issue with the types package (@types/koa), not Koa itself. If the types were updated to reflect the change made in #1474, there would no longer be an issue.

@mattbasta
Copy link
Author

@stephenmathieson The issue is that the type signature changes depending on your bundler (or, what imports the library). Right now, depending on how you use the same module, you may get one of two different behaviors. The types represent only one of those two behaviors.

That's not to say the types are correct, but I'd argue that Koa's current configuration makes it hard (if not impossible) to get that right.

@stephenmathieson
Copy link

stephenmathieson commented Dec 28, 2020

Right now, depending on how you use the same module, you may get one of two different behaviors

Yes, one CommonJS, one ESM. That is by design (I think 🤔). I disagree that the two should be consistent. CommonJS users don't need .default, so providing it doesn't seem to make sense.

I very well may be missing something here, but I still believe this would be resolved if @types/koa were updated to support the newly added MJS export.

@mattbasta
Copy link
Author

The issue is that we're talking about three separate systems, which are independent:

  1. The types
  2. The module formatting system
  3. The import syntax

You can be using ESM imports in your code while still using CommonJS (this is extremely common with Webpack and other bundlers). Your bundler may use traditional CommonJS modules and MJS modules simultaneously (as is the case with recent versions of Webpack). This means that the version which gets imported and compiled is not necessarily known.

Regardless of what the type definitions say, at this point, Koa may be exposed as a default import or not. There is no way to create type definitions at this point which describe the shape of Koa's exports, since they change depending on the configuration of the system which imports the files (Webpack, directly with Node, etc.) and the tsconfig.json file (whether TS is configured to export import statements or CommonJS require() calls).

Using ESM imports is extremely common even under CommonJS because it enables tree shaking and other features. Because of a change in a project's environment (a version change, dependency changes), regardless of type definitions, Koa can spontaneously stop working because the MJS version might become enabled (or disabled!). Because there is no overlap in the shape of the two versions, it's impossible (without hacks) to write an ESM import of Koa which works under both MJS and CommonJS.

Consider a more direct scenario: I decide to write a framework which wraps Koa. It's now impossible for me to (without hacks) write the MJS version first and compile it to CommonJS. This is because if I write import Koa from 'koa'; in my MJS version, that will get transpiled to require('koa').default for the CommonJS version, which will fail to import. I'm forced to have separate handwritten versions of my code which import Koa differently.

While I understand that semantically it makes little sense to have a default export for CommonJS with require() calls, it does have a substantial impact for folks using ESM with CommonJS.

@stephenmathieson
Copy link

I see, thanks for the explanation. JS is getting out of hand, haha.

Let's see what the maintainers want to do about this.

@miwnwski
Copy link
Member

miwnwski commented Jan 4, 2021

So I've read and re-read your comment @mattbasta. I am not at all a TS user, but if Application simply exposed a static default property referencing Application (circular), wouldn't it solve the typings issue?

Off the top of my head, here, untested:

module.exports = class Application extends Emitter {
  static get default = Application
}

Edit:
Well, since we need to avoid breaking v2, I guess static get default () { return Application } would be more appropriate ..

@mattbasta
Copy link
Author

@miwnwski That would solve the runtime issue, yes. The types would need to be updated to reflect that also, but that should be trivial.

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

No branches or pull requests

3 participants