Skip to content

Refactor: convert to ESM module (.js files) #19719

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

Closed
mhegazy opened this issue Nov 3, 2017 · 9 comments
Closed

Refactor: convert to ESM module (.js files) #19719

mhegazy opened this issue Nov 3, 2017 · 9 comments
Labels
Domain: Refactorings e.g. extract to constant or function, rename symbol Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Nov 3, 2017

  • var mod = require('mod'); =>

    • if mod is callable then import mod from 'mod'
    • else mod is not callable the import { a, b } from "mod" and replace mod.a and mod.b
    • namespace import should ever be used?
  • var mod= require('mod').a; => import { a } from "mod";

  • const { a } = require('mod'); => import { a } from "mod";

  • if (__DEV__) { var warning = require('warning'); }

    • disallowed?
    • have two actions? one sync and one async?
  • function f() { var warning = require('warning'); } => async function f() { var warning = await import('warning'); }

    • maybe do nothing here as well
  • module.exports = EventPlugin; => export default EventPlugin;

  • fix imports in other files
  • should we also fix require("mod") to require("mod").default?
  • module.exports.blah = 0 => export const blah = 0

    • should we support this too
      var myModule = module.exports = exports = {};
      myModule.a = 0;
      
    • this is another common pattern in CJS modules that we should consider supporting
    • we already have handling for this in the binder in .js files
  • module.exports = require('mod'); => export * from 'mod';

  • module.exports = {foo: function () { } ... } => export function foo() {} ?

    • is this even common enough to support?
@mhegazy mhegazy modified the milestone: TypeScript 2.6.2 Nov 3, 2017
@mhegazy mhegazy assigned ghost Nov 3, 2017
@mhegazy mhegazy added Suggestion An idea for TypeScript Domain: Refactorings e.g. extract to constant or function, rename symbol labels Nov 3, 2017
@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 7, 2017

Of the last two, perhaps the first should be

module.exports = require('mod');
    // => 
       export * from 'mod';
       export {default} from 'mod';

since the export * should exclude default.

And perhaps the second should be

module.exports = {foo: function () { } ... };
    // =>
        export default {foo: function () {}};

as, if the module is consumed natively as an ESM by a NodeJS ESM, the named export foo would not be available at all otherwise.

@ghost
Copy link

ghost commented Nov 10, 2017

@aluanhaddad I think export * already includes all exports? At least TypeScript treats it that way.

@aluanhaddad
Copy link
Contributor

I don't believe that's always the case. It doesn't appear to be true for Babel's
CommonJS output and TypeScript's System.register transform also behaved differently the last time I checked.

@ghost
Copy link

ghost commented Nov 10, 2017

Just tested with .mjs and it looks like you're right and default isn't brought along. @rbuckton Can you confirm that this is the correct behavior? Should TypeScript be catching that error?

@ghost
Copy link

ghost commented Nov 10, 2017

@aluanhaddad Can you clarify what you mean by the second comment? If I write export function foo() {} I can access it from a .mjs file just fine with import { foo } from "./a";.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 10, 2017

Can you clarify what you mean by the second comment? If I write export function foo() {} I can access it from a .mjs file just fine with import { foo } from "./a";.

Yes, I did not mean to imply otherwise. All exports (except the default export if present) are reexported by export * from "m";

Here is the specification text that describes the default exclusion.

https://tc39.github.io/ecma262/#sec-getexportednames

See section 7.c.i

@ghost
Copy link

ghost commented Nov 10, 2017

By second I meant the one starting with And perhaps the second should be.

@rbuckton
Copy link
Member

@aluanhaddad is correct, default is excluded from re-exports using export * from ... syntax.

@aluanhaddad
Copy link
Contributor

@Andy-MS

By second I meant the one starting with And perhaps the second should be.

Ah, sorry, my brain kind of went to sleep for a second 😪.

I meant that since Node.js is not yet supporting named exports, unless something has changed, and since this concerns a refactoring transformation that might be output using --module es2015, it might make sense for the refactoring to place properties on the default export instead of creating named exports.
I'm not sure about this since it depends on how the imports are transpiled if at all.

module.exports = {foo: function () { } ... };
    // =>
        export default {foo: function () {}};

@mhegazy mhegazy added this to the TypeScript 2.7 milestone Jan 9, 2018
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 9, 2018
@ghost ghost closed this as completed in #19916 Jan 9, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Refactorings e.g. extract to constant or function, rename symbol Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants