Directory *imports* throwing in ESM node

[Andarist]

How to reproduce:

1. Add an entrypoint: "preconstruct": { "entrypoints": [ "index.ts", "foo.ts"] }
2. packages a library, so the entrypoint becomes a "proxy" directory with package.json inside it
3. attempt to import that from node using native ESM implementation there: import { foo } from "pkg/foo";
4. end up with an error:

Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/some-app/node_modules/pkg/foo' is not supported resolving ES modules imported from /some-app/index.js

Repro:

https://github.com/markdalgleish/preconstruct-node-esm-import-issue/tree/f181ab2243c98255723ee106ae665df1fc590db6

Possible solution:

Emit package.json#exports with the appropriate fields for the declared entrypoints. If we do this just for CJS, without attempting to support ESM bundles in node then it should work and not break anything (like with everything related to this... we should still be extra careful about this though).

This would limit what can be imported/required in node from a package. However, the whole point of Preconstruct is to only allow what is declared so if somebody has been reaching into "internal" files that's kinda on them. This could be added under an experimental flag if we are worried about the unwanted side-effects. Optionally we could consider always adding package.json to the exports as that seems to be a pretty common requirement and I don't see why somebody would really want to "hide" this file.

cc @markdalgleish

[emmatown]

I'd definitely be down with an experimental flag to add this. I wouldn't want to force it since I don't think there's a reasonable way of adding an exports field that wouldn't be a major for a package in the general case but we would probably make adding the exports field the default in a future major of Preconstruct with an opt-out.

For the linked repo, adding this to the package.json in @vanilla-extract/sprinkles seems to work

"exports": {
  "./package.json": "./package.json",
  ".": {
    "module": "./dist/vanilla-extract-sprinkles.esm.js",
    "default": "./dist/vanilla-extract-sprinkles.cjs.js"
  },
  "./createRuntimeSprinkles": {
    "module": "./createRuntimeSprinkles/dist/vanilla-extract-sprinkles-createRuntimeSprinkles.esm.js",
    "default": "./createRuntimeSprinkles/dist/vanilla-extract-sprinkles-createRuntimeSprinkles.cjs.js"
  },
  "./createUtils": {
    "module": "./createUtils/dist/vanilla-extract-sprinkles-createUtils.esm.js",
    "default": "./createUtils/dist/vanilla-extract-sprinkles-createUtils.cjs.js"
  }
},

Note the module rather than import condition since the ESM isn't intended for Node.

One thing we should check is, do bundlers still respect the browser field when using something from an exports field or would we have to add browser conditions for packages that do a browser field?

Also with the whole TypeScript Node ESM stuff, do we need a types condition or will TypeScript be fine with the .d.ts file next to the .cjs.js file? Also kinda random but just a thing I've been thinking about is that when the TypeScript Node ESM support is released and if people use an exports field, I'm guessing people will run into issues like this:

// packages/pkg-a/src/a.ts (is not an entrypoint)
export type X = {
  x?: X;
};

export function x(): X {
  return {};
}

// packages/pkg-a/src/index.ts (is an entrypoint)
export { x } from "./x";

// packages/pkg-b/src/index.ts (is an entrypoint)
import { x } from "pkg-a";

export const a = x();

I would expect TypeScript to fail when generating the declarations for packages/pkg-b/src/index.ts since it can't reference X if it's not exported by anything in the exports field. The answer will probably be "you need to export your types from an entrypoint". IMO this error is Good Actually since it'll force consumers to not depend on the dist/declarations structure but it'll probably be painful. I suppose technically you could write export const a: ReturnType<typeof x> = x(); but I can't imagine a package author preferring users to do that over exporting the types.

Config thoughts

I would be totally fine with an implementation that doesn't look like this to start and ofc I'm keen to hear thoughts/suggestions on this design/alternatives.

Opt-in on a project level or package level basis. For now, this would be in addition to enabling an experimental flag for it.

"preconstruct": {
  "exports": true
}

And Preconstruct would completely control the exports field for a package with fix and validate and disallow extra fields so that if you remove an entrypoint for example, it can remove the field from exports. To add extra fields, we would allow:

"preconstruct": {
  "exports": {
    "extra": {
      "./blah": "./blah.js"
    }
  }
}

And ideally it could deeply merge conditions.

I'm thinking that if exports hasn't been enabled and a package has an exports field, Preconstruct would ignore it though not totally set on that.

[Andarist]

I wouldn't want to force it since I don't think there's a reasonable way of adding an exports field that wouldn't be a major for a package in the general case

what kind of breaking changes this can introduce if this doesnt try to introduce ESM support in node?

One thing we should check is, do bundlers still respect the browser field when using something from an exports field or would we have to add browser conditions for packages that do a browser field?

I have been testing this recently with @rollup/plugin-node-resolve and IIRC we have to add browser condition to the exports field.

Also with the whole TypeScript Node ESM stuff, do we need a types condition or will TypeScript be fine with the .d.ts file next to the .cjs.js file?

We’d have to test with the @next release channel of TS. This wont matter at least until 4.6 gets released since the support for exports has been disabled in 4.5. If we could already output something compatible with future release of TS - then definitely we should try to do that.

I would expect TypeScript to fail when generating the declarations for packages/pkg-b/src/index.ts since it can't reference X if it's not exported by anything in the exports field

What is emitted for that today? Maybe type-level import() wont be limited by exports the same way as runtime is? If this would fail is gonna become a real PITA for users as we dont usually think about if the inferred type can be emitted based on public exports of other pkg. Either way - this seems to be a TS issue and not a Preconstruct one.

And Preconstruct would completely control the exports field for a package with fix and validate and disallow extra fields

Agreed, but i think that package.json should always be exported.

To add extra fields, we would allow:

I would skip that for now - it sounds niche, potentially YAGNI

I'm thinking that if exports hasn't been enabled and a package has an exports field, Preconstruct would ignore it though not totally set on that.

Sounds good to me - if users dont give us control over exports then we should simply ignore it, they might want to manage it themselves.

[emmatown]

what kind of breaking changes this can introduce if this doesnt try to introduce ESM support in node?

The general "i was importing x, x isn't in the exports field" which of course is a matter of interpretation as to whether importing some file that isn't in the added exports field is a breaking change or not since if it's not in the exports field, it was probably never intended to be public and some may see it a breaking change and some may not. I don't think Preconstruct should potentially force a package author to do a major if they think adding an exports field would be major. This is similar to how the dist file names have a way to go back to the old names if a package author thinks it would be breaking to change it and doesn't want to do a major.

What is emitted for that today? Maybe type-level import() wont be limited by exports the same way as runtime is? If this would fail is gonna become a real PITA for users as we dont usually think about if the inferred type can be emitted based on public exports of other pkg. Either way - this seems to be a TS issue and not a Preconstruct one.

On 4.5, you would get a import to pkg-a/src/a. (or similar but pointing to declarations)

On the latest nightly as of this comment (4.6.0-dev.20211126), you get something like this:

src/index.ts:2:14 - error TS2742: The inferred type of 'a' cannot be named without a reference to '../node_modules/blah/src/a'. This is likely not portable. A type annotation is necessary.

2 export const a = x();
               ~

(I didn't test exactly the example shown above but the same general thing)

To be clear, I don't think this is something that should be "fixed". The current status quo of packages easily having implicit dependencies on their dependencies internal module structure imo is a worse problem than breaking once(or not at all if you don't need to generate declarations that need to reference something that isn't exported). Getting an error upfront and being able to address it is much better imo than some package changing their internal structure and your package that depends on it breaking.

Agreed, but i think that package.json should always be exported.

Yeah

I would skip that for now - it sounds niche, potentially YAGNI

I agree that it can be skipped it for now but I don't think it's that niche, having some other js/json file or etc. that you want to allow people to import from and isn't managed by Preconstruct seems like a reasonably common use case. (Emotion does that for example)

[Andarist]

src/index.ts:2:14 - error TS2742: The inferred type of 'a' cannot be named without a reference to '../node_modules/blah/src/a'. This is likely not portable. A type annotation is necessary.

Eh, that could really use a suggestion from TS on how to fix it 🙄 I expect many people to trip over this.

To be clear, I don't think this is something that should be "fixed".

Yeah, I got your intention - was just wondering what are the exact implications. Nice catch on this one.

I agree that it can be skipped it for now but I don't think it's that niche, having some other js/json file or etc. that you want to allow people to import from and isn't managed by Preconstruct seems like a reasonably common use case. (Emotion does that for example)

IIRC Emotion only "exports" macros this way - and those entrypoints could be easily included in ./src

[nicksrandall]

I extended PR #435 to also support package exports. I used the vanilla-extract repo to test and I can confirm it is working.