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, 9/22/2021 #46012

Closed
DanielRosenwasser opened this issue Sep 22, 2021 · 12 comments
Closed

Design Meeting Notes, 9/22/2021 #46012

DanielRosenwasser opened this issue Sep 22, 2021 · 12 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Binding Pattern Breaking Changes

#45846 (comment)
#46009

  • Motivation

    declare function g<T>(): T;
    
    // Type is { x: any, y: any, g: any }
    // which is kind of dumb.
    const { x, y, z } = g();
  • Other more complex issues.

  • Why?

    • Best thing we can imagine is left-to-right completions for

      const { x, y, z } = { /*|*/ }
      //                      ^
      // We expect completions for x, y, z.
  • Current proposal is to disable for object binding patterns, but re-enable contextual typing from array binding patterns.

  • We still don't fully understand the inferences - probably best to do that.

  • When we started doing implied types from binding patterns, we didn't have types apart from any - now have never and unknown.

  • Let's do a full revert and think through this a bit more.

Node ESM Support

#44501
weswigham#67
weswigham#68

  • New resolution modes: node12 and nodenext

  • CJS and ESM files can resolve files via different paths based on the syntax and containing file.

    // Always follow CJS resolution
    import foo = require("foo")
    
    // Dynamic import - always follow ESM resolution
    const a = import("foo");
    
    // Always depends on the **containing file**!
    import * as a from "foo";
  • This is "solved" albeit complicated, but what is not solved is...

  • Import type syntax:

    type Foo = import("foo").Foo;
    • Can't disallow in CJS files because of existing CJS .d.ts files.
  • So how do you differentiate between "this should resolve as ESM vs CJS"?

    • Don't have any other syntax for inline type import syntax.
  • Could have a different conditions passed into import(...) types.

    • Like in an import assertion: import("foo", { /*conditions*/ })
  • Why does this matter?

  • Technically the MVP doesn't need to include this...but it will likely be an issue.

  • If you need to reference a type in ESM, either in .d.ts emit or by-hand, can you just write

    import "esm-file";
    • No, you can't :(
  • Can you lean on import() types always getting inferred from containing context?

    • Yes, you have to for compat, but you can't model a CJS file that actually import()s an ESM.
  • Import assertions affecting module resolution?

    • Seems like a no-no, but might need to use sibling conditions as a way to bypass that.

      type Foo = import("foo", { assert: { }, resolver: "esm" });
  • Heavy regret of not introducing require() types; now you need a "real import type" type.

  • Whole thing is such a 🤬😫😢😭

  • What if we just didn't let you write import in a non-module file?

    • Force users to write require() in CJS
    • Goes against a lot of expectations.
    • Isn't that what people want to write though?
    • What is the guidance? Users should use module: node12, but as far as they're concerned they're still doing CJS.
  • If our heads are spinning from this, users' will too.

    • A lot of concerns around complexity.
  • Stripping out complexity?

    • Export maps are complicated, but are a necessary feature for bridging the ESM and CJS worlds.
      • Trying to get it set up is rocket science, not supporting it would be worse.
  • "maybe I'm just old and grumpy"

    • "no this sucks"
  • We should be able to explain the happy path to people.

  • We have a lot of concern over how easily we'll be able to communicate best practices here; this is not just something that can affect TypeScript, the complexity can significantly hurt Node.js too.

@Jamesernator
Copy link

Export maps are complicated, but are a necessary feature for bridging the ESM and CJS worlds.

  • Trying to get it set up is rocket science, not supporting it would be worse.

This could be considerably easier if Node actually exposed the resolvers as an API rather than every tool having to reimplement the resolver logic. There have been comments around on Node issues about people being interested, but it might be worth making an explicit issue.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Sep 23, 2021

I think given the fact that TypeScript has to lay its own .d.ts resolution on top of Node's resolution means that that was an inevitability anyway.

But the complexity is more about supporting users, not supporting the logic ourselves. We can barely keep the whole thing in our heads, but we've spent years thinking about it, so we think there's a very small chance that a user would figure out the correct setup quickly on their own.

@andrewbranch
Copy link
Member

andrewbranch commented Sep 23, 2021

I think the notes lost some context there—what I called “rocket science” was actually hand-authoring type definitions (e.g. for DefinitelyTyped) that work for a package that ships CJS and ESM side-by-side, and my point was that being able to redirect type resolution (to a separate module definition for each module kind) via export maps is going to be a super nice feature for us, and for type definition authors.

(Also, rocket science was probably a bad analogy, since I assume that rocket science is meticulous, precise, and reliable 😬)

@DanielRosenwasser
Copy link
Member Author

Sorry @andrewbranch, that's what the notes were meant to reflect, but the convo was happening a bit too fast.

I think the analogy was fine. Usually the idiom "it's not rocket science" implies "this is simple, so nobody should get overwhelmed by it" whereas this is something we don't think is simple, and thus can make no guarantees about whether anyone will be overwhelmed by it 😅

@robpalme
Copy link

Force users to write require() in CJS

That seems fine IMO.

If users adopt the new "node12" mode, they are opting into modern Node which has two resolvers. Being obliged to be explicit on type resolution seems a small price to pay for the win on clarity-of-intent for anyone reading the code, hopefully reducing head-spinning.

It places the migration costs on the old code (which could be as simple as a one-time quick-fix), rather than polluting new code with oddities like import assertions.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Sep 24, 2021

Being obliged to be explicit on type resolution seems a small price to pay for the win on clarity-of-intent for anyone reading the code, hopefully reducing head-spinning.

I kind of agree with this, and I don't think I was able to articulate @weswigham's response to this one.

It sounded like the issue was with forcing users to migrate old code, but I think that old code has to get migrated as one big bang anyway, right? You can't have a sane story where you progressively convert some of your modules in an existing TypeScript codebase from CJS to ESM unless you start at the roots and switch your import statements to import = require() anyway, right?

@DanielRosenwasser
Copy link
Member Author

But there's a big "what is the default configuration?" story. The whole issue is that a TypeScript file today allows ESM import syntax. If we ever picked node12 as the default, but forget to write "type": "module" in your package.json, then as soon as you write an import statement, you've screwed up. I guess it's the same for .js files too, so maybe that's okay? 😬

@robpalme
Copy link

I guess it's the same for .js files too, so maybe that's okay?

I like that line of thinking. If "node12" mode meant "no module transforms for CJS files" it gets us closer to the world of TS being JS + types. Which I think is a useful north-star principle for side-stepping the complexity being discussed/proposed in this thread.

My instinct says the one-time big-bang migration this imposes, at the point the project enables "node12", is worth it for the goal of long-term simplicity. Users that want faux-ESM (ESM that is transformed to CJS) will always be able to use the existing "commonjs" mode.

Would love to hear more from Wes about his original response.

@weswigham
Copy link
Member

No - people have literally been asking for TS to be the tool that people lean on to ease the transition - not the thing that people use to be punished for node's support of ESM by being forced back into using old syntax. Many times I've been told "yeah, node's dual module strategy will be bad because technical reasons, but TS will bridge the gap, right?". There is no "big bang migration" caused by the new mode - in fact, there's likely no migration required at all for existing code. People have been using TS targeting cjs output for almost forever - having a modern version of TS's "node mode" break that behavior is a massive break in expectations. A key and very useful feature of the new node resolution is that, largely, if you have an existing moduleResolution: node project, it'll behave the same way under module: node12 until you start progressively adopting new node resolver features. (Eg, export maps, conditional exports, type: module, cts and mts and the like).

I don't get why so many people central to parts of the node package community want to punish existing node package authors with forced migration. It's a nongoal for me.

@weswigham
Copy link
Member

Also, that whole discussion on import assertions is about declaratiom emit and isn't a problem in non-declaration code. And it's a problem in declaration emit because we only have one type-space construction to look up package types import("foo").A, but the specifier can now resolver to two (or more) unique types, and we have no way to configure which a given import type resolves to. For now, we can shrug and say "figure it out yourself: write a type annotation", and that was our conclusion - I just wanted to get the team thinking about what it would take to provide an import type construction capable of configuring the resolution used to find the types in the future, since, just like import types originally, it'll probably be a pretty consistent community ask. Probably.

@robpalme
Copy link

Thanks Wes - so it's only for declaration code, and only for import types, where there's no current syntax to explicitly select a resolution.

I think your first comment explains the justification for staying in the business of module transforms.

@treybrisbane
Copy link

Having been loosely keeping up with the TypeScript discussions around the new Node resolutions, I just wanna say thanks for putting in what is clearly a significantly amount of time and effort towards both getting things right and making things as user-friendly as possible! 😀

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

6 participants