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, 5/14/2021 #44097

Closed
DanielRosenwasser opened this issue May 14, 2021 · 7 comments
Closed

Design Meeting Notes, 5/14/2021 #44097

DanielRosenwasser opened this issue May 14, 2021 · 7 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

ES Modules in Node

https://gist.github.com/weswigham/22a064ffa961d5921077132ae2f8da78

  • export maps and .d.ts files

    • The same way we let you file a types field to reflect the main, we should have a "types" condition in your export maps.

      {
          "exports": {
              "./": {
                  "import+types": "./ts/esm/index.d.ts",   // <- this reflects the types you get for an import
                  "require+types": "./ts/cjs/index.d.ts",  // <- this reflects the type you get for a require()
                  "import": "./esm/index.js",
                  "require": "./cjs/index.js",
              }
          }
      }
    • Alternatively, "nested conditions" approach

      {
          "exports": {
              "./": {
                  "types": {
                      "import": "./ts/esm/index.d.ts",   // <- this reflects the types you get for an import
                      "require": "./ts/cjs/index.d.ts",  // <- this reflects the type you get for a require()
                  },
                  "import": "./esm/index.js",
                  "require": "./cjs/index.js",
              }
          }
      }
  • typesVersions?

    • Version specifier in the exports map?

      {
          "exports": {
              "./": {
                  "import+types>4.4": "./ts/esm/index.d.ts",
                  "require+types>4.4": "./ts/cjs/index.d.ts",
                  "import+types": "./ts4.4/esm/index.d.ts",
                  "require+types": "./ts4.4/cjs/index.d.ts",
                  "import": "./esm/index.js",
                  "require": "./cjs/index.js",
              }
          },
      }
      • Keeping it like this has the benefit of keeping things up to date.
  • Do we expect these exports to be written by hand?

    • Generally, yes.
  • This is all quite a lot of complexity.

    • Most people won't need multiple entry points and the like.
  • typesVersions was explicit, this isn't.

  • It's not necessarily just about ESM imports vs CJS require.

    • It's also about Node vs. the browser.
      • Also maybe about ES5 vs ESNext
    • There's value in the fact that you can nest these in flexible ways.
    • import+types>4.4 seems a little bit confusing, maybe "abusive" of the export map syntax.
    • Spec kind of expected identifier-like strings.
      • This behavior is relied on specially - for example, anything starting with a . is a path, anything not is "special".
    • Make sure people involved in Node take a look.
    • (side note: be cognicent of people importing .d.ts files like assets?)
  • Seems like team slightly prefers nesting, not concatenating (e.g. yadda+types)

    {
        "exports": {
            "./": {
                "types": {
                    "import": "./ts/esm/index.d.ts",
                    "require": "./ts/cjs/index.d.ts",
                },
                "import": "./esm/index.js",
                "require": "./cjs/index.js",
            }
        }
    }
    • But still need to check how that works for versioning.
    • Versioning?
      • typescript@>=4.4

      • types>=4.4

      • What about

        {
            "exports": {
                "./": {
                    "types": {
                        "import": "./ts/esm/index.d.ts",
                        "require": "./ts/cjs/index.d.ts",
                    },
                    "typesVersions": {
                        ">=4.4": "..."
                    },
                    "import": "./esm/index.js",
                    "require": "./cjs/index.js",
                }
            }
        }
        • Doesn't make sense because there's multiple ways to specify versioning in this manner.
    • If you care about versions older than whatever TypeScript supports this, you'll still need a top-level typesVersion.
    • Can't omit support for typesVersions from the MVP, otherwise users have to specially support the MVP in a weird way.
    • Leaning towards
      {
          "exports": {
              "./": {
                  "types": {
                      "import": "./ts/esm/index.d.ts",
                      "require": "./ts/cjs/index.d.ts",
                  },
                  "types<=4.5": {
                      "import": "./ts/esm/index.d.ts",
                      "require": "./ts/cjs/index.d.ts",
                  },
                  "import": "./esm/index.js",
                  "require": "./cjs/index.js",
              }
          }
      }
    • "types<=4.5" or "types@<=4.5"?
      • types@* vs types*?
        • You'd just write types.
        • But it's usually a semver pattern.
        • Need something that separates the semver pattern.

Breaking Changes

#44093

  • We'll look into this.

#44092

  • Change in inference.
  • We'll look into this.

#44087

  • Issue is that we look for all things that number is assignable to, reduce everything.
  • In theory, should reject the assignment.
    • So inclined to say "won't fix" - weirdness is caused by a bad assignment.

#43877

  • Bloomberg found the cause internally.
  • Without a minimal repro, can't improve anything.

unknown in catch

#41016
#41013

  • People liked unknown in a survey.
  • But it sucks to turn on in the TypeScript codebase.
    • What do you mean by it "sucks in the TS codebase?"
    • Lots of well-working code that doesn't improve.
    • "I'm willing to do the feature, I'm not willing to be the one who converts the TypeScript codebase to use it."
      • Like all good strictness flags.
  • Seems wrong that we don't complain about that loose any under strict.
  • Can always do a refactoring to migrate an existing codebase to use : any in catch clauses.
    • That helps us feel okay with putting this under --strict.
  • Would be great to have a way to say "the only thing this throws is Error".
    • Agreed, but no way to do that today.
    • Could totally imagine a thing that asserts the type of the error thrown.
  • Seems like we're okay with this flag going under --strict.
@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label May 14, 2021
@milesj
Copy link

milesj commented May 14, 2021

In regards to exports, is the import/require differences based on the source code? With TypeScript, people are almost always using import/export, so wouldn't it always just be import+types? What situation would require+types pop up?

Also, for catch clauses. I've always felt like it should infer to Error if no type is provided, otherwise use unknown if explicitly provided. I'm not sure how sound that would be though.

// Default type `Error`
try { } catch (error) {}

// Explicit `unknown`, requires type guards
try { } catch (error: unknown) {}

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented May 14, 2021

@weswigham might have opinions here, but my understanding is that you would potentially want to have different behavior between

import * as foo from "foo";

and

import foo = require("foo");

Additionally, I think that we would disallow mixing these import forms outside of declaration files.

@weswigham
Copy link
Member

is the import/require differences based on the source code?

Yes. An es6 like import or dynamic import sets the import condition, a require sets the require condition. This means, notably, await import("whatever") and require("whatever") in the same file, can resolve to different things with different shapes.

@chyzwar
Copy link

chyzwar commented May 19, 2021

Do you plan to support conditional exports based on target env: browser/node and runtime development/production?
https://webpack.js.org/guides/package-exports/

@cspotcode
Copy link

In the examples, ./esm and ./cjs subdirectories are used for 2x versions of the module, but the contents of both use .js file extension. Does this imply that one has a package.json in it?

For example:

// package.json
{
    "name": "libfoo",
    "type": "module",
    "exports": {
        "./": {
            "import": "./esm/index.js",
            "require": "./cjs/index.js",
        }
    }
}
// ./cjs/package.json
{
  "type": "commonjs"
}

If not, I believe node will throw an ERR_REQUIRE_ESM when attempting to require('libfoo') because ./cjs/index.js will be loaded as ESM. If yes, and if ./cjs/package.json does not declare dependencies, then I think imports will fail in yarn2 PnP.

@weswigham
Copy link
Member

Does this imply that one has a package.json in it?

Yes.

@arcanis
Copy link

arcanis commented May 28, 2021

If yes, and if ./cjs/package.json does not declare dependencies, then I think imports will fail in yarn2 PnP.

I think it should work. We always get dependencies from the "true" package root (you can't declare dependencies from the package's subfolders), and only use nested package.json for the purpose of resolving imported paths extensions or exports fields (just like Node).

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