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

Design Meeting Notes, 4/20/2022 #48790

Closed
RyanCavanaugh opened this issue Apr 20, 2022 · 3 comments
Closed

Design Meeting Notes, 4/20/2022 #48790

RyanCavanaugh opened this issue Apr 20, 2022 · 3 comments
Labels
Design Notes Notes from our design meetings

Comments

@RyanCavanaugh
Copy link
Member

  • import emit in module: none Emit for dynamic import (import()) when target >= ES2020 and module == None #48702

    • Today, import in module: none + outFile produces an async require
    • This makes no sense per the config flags
    • But it could work, so someone might be doing it anyway
    • Seems like this is just a bug, but there may be people depending on it
    • There's no combination of flags that would allow this if we "fix" it
    • Q: ES2020 is the first version that supports dynamic import? A: Yes
    • What is module: none supposed to do?
    • We used to emit CommonJS because we had to do "something", and this requirement predated ES modules
    • How does this scenario work if we transition the TS codebase to modules?
      • It should be OK since we can emit to nodenext
    • Q: Do we have any untransformed imports today? A: No
      • Q: Is potentially this a problem? A: Not today (used to be)
    • What problem are we trying to solve and in what scope?
      • Is it actually a 4.7 deliverable?
        • No (multiple reasons)
      • Do we need to just do a one-off hack here?
    • Q: Can we use nodenext module target? A: No, because we use outFile
    • We handled dynamic import wrong, basically
    • Options on the table
      • "Just fix this"; potentially break some builds. Broken builds can use require instead
      • Build-time hack (e.g. write IMPORT, change to import)
      • New (undocumented) flag
    • Q: Why not provide this behavior under commonjs?
      • Specifying module and outFile simultaneously is an error because it's a really dangerous configuration
    • Posited: We should do other fixes in module: none, e.g. handling import declarations, in 4.8
    • Consensus: "Just fix this", in 4.8 branch (to avoid breaks in RC period)
    • If VS Code needs something in release before 4.7, we can buildhack
  • Parser Problems allow PrivateIdentifier in QualifiedName #48640

    • We added support for typeof a.#foo
    • The PR had an incorrect type assertion
    • API change: QualifiedName now includes MemberName instead of Identifier
    • We could change the typeof operand type instead?
      • Agreed
  • Problems with Parsers [4.7-beta] Parsing failure for arrow function expr in conditional expr #48733

    • (false ? (param): string => param: null);
    • Is the : a type annotation, or the colon in the form x ? y: z ?
    • The former turns out to be pretty common
    • The parser has a heuristic based on looking two (?) tokens ahead, but this is insufficient
    • The code a ? (n) : m => expr is ambiguous until you get to the end of the expr and see that the : is missing
    • This is an unbounded lookahead
    • a ? (b) : c => (d) : e => f means what ?
    • Proposal: Parentheses policing preferred, per personnel's palaver
    • We parse this wrong in JS
    • Some cases to consider for consistency
      • Unambiguous (?): a ? (): b => c : d
      • Ambiguous (?): a ? (x): b => c : d
      • Unambiguous (?):a ? (x: y): b => c : d
      • Ambiguous (?): a ? (x, z): b => c : d
    • Put up an "expression-preferring" Playground and try it out
@RyanCavanaugh RyanCavanaugh added the Design Notes Notes from our design meetings label Apr 20, 2022
@jakebailey
Copy link
Member

jakebailey commented Apr 21, 2022

@fatcerberus
Copy link

fatcerberus commented Apr 21, 2022

Re: module: none, my intuition was always that this implied you wanted to write script code and that therefore any module constructs such as (static) import or require() would be errors. However, dynamic import is always allowed, even in scripts, so IMO should not produce an error, but also should not be transformed to a require() unless module is explicitly set to CommonJS.

This is tricky though because dynamic imports in script code don't treat relative paths the same as those in module code at runtime. (If a relative path works at all, it would be relative to e.g. the current URL).

@DanielRosenwasser
Copy link
Member

@fatcerberus I think that resonates with me too - but then we also require a specific target for import() calls as well if you're not in certain module modes, right? And it sounds like TypeScript targets ES5 right now, but wants to use import(), right?

(@rbuckton)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

4 participants