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

Add "types" entries to the "exports" property of package.json, to allow consumption from Typescript with the new Node16 module resolution strategy #1217

Closed
Duncan3142 opened this issue May 17, 2022 · 16 comments · Fixed by #1307
Assignees

Comments

@Duncan3142
Copy link

Duncan3142 commented May 17, 2022

Is your feature request related to a problem? Please describe.

I'm unable to consume "@graphql-yoga/common" from Typescript 4.7, when module resolution is set to Node16

Describe the solution you'd like

Add "types" entries to the "exports" property of package.json

Describe alternatives you've considered

None

Additional context

The following should suffice

"exports": {
  ".": {
    "types": "./index.d.ts",
    "require": "./index.js",
    "import": "./index.mjs"
  },
  "./*": {
    types": "./*.d.ts",
    "require": "./*.js",
    "import": "./*.mjs"
   },
  "./package.json": "./package.json"
}
@ardatan
Copy link
Collaborator

ardatan commented May 25, 2022

Could you create a reproduction on CodeSandbox or StackBlitz? Thanks

@Duncan3142
Copy link
Author

I've created a Github repo

https://github.com/Duncan3142/yoga-node16

@Duncan3142
Copy link
Author

Duncan3142 commented May 28, 2022

Is that sufficient?

See the following links for more information:

Typescript 4.7 File Extensions
Typescript 4.7 Package Imports/Exports
Node package docs

@Duncan3142
Copy link
Author

Duncan3142 commented May 28, 2022

It might be necessary to have two sets of files for "@graphql-yoga/*" packages, one for commonjs and one for esm, and seperate package.json files. This is due to the dependence on "@repeaterjs/repeater", which is a "type": "module" package.

"@repeaterjs/repeater" itself follows this pattern.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 4, 2022

Hey @Duncan3142, I created #1258 for this. However, this will require upstream changes within bob (the tool we use to bundle our code). I created an issue over here: kamilkisiela/bob#39

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 20, 2022

Hey @Duncan3142, I created this PR on your repository: https://github.com/Duncan3142/yoga-node16/pull/1

Currently, I also had to change the tsconfig to not typecheck libs as @repeaterjs/repeater is not yet compatible with nodenext...

The PR on yoga is #1307
Released canary versions: #1307 (comment)

Please let us know whether this resolves the problem

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 21, 2022

Status sync update:

In https://github.com/Duncan3142/yoga-node16/pull/1#issuecomment-1160407437 @Duncan3142 mentioned that he suspects that the type definitions the bob-the-bundler canary (v2.0.0-canary-730aea6.0) built from kamilkisiela/bob#55 produces are invalid.

I set up patch-package for the sample project provided and altered the type definitions as instructed (.d.mts) (https://github.com/Duncan3142/yoga-node16/pull/1#issuecomment-1161297244), without success.

Now, we are waiting for @Duncan3142 (https://github.com/Duncan3142/yoga-node16/pull/1#issuecomment-1161302673), to adjust the patch-package setup so the type definitions are legit and the TypeScript compilation succeeds.

@Duncan3142
Copy link
Author

Duncan3142 commented Jun 21, 2022

@n1ru4l I've created patch-package fixes in https://github.com/Duncan3142/yoga-node16/commit/3c20f6a77d876b1afdb1d494cd9f63c68e6e841d which hopefully makes the problem clear.

I had to make changes in the @envelop/core package too

The changes basically involved adding "types" entries to the "exports" property of the affected package.json files, and creating .d.mts files where necessary.

I switched from Yarn 3 to NPM cause patch-package doesn't seem to work with Yarn 3 lock files.

The changes which best illustrate the issue are in '@graphql-yoga/subscription/index.d.mts'. Because its a .d.mts file, I've had to use explicit extensions for relative imports, but note that I only had to use the .mjs extension for files which directly or indirectly imported '@repeaterjs/repeater'

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 21, 2022

Can you document the key differences to your solution compared to https://github.com/Duncan3142/yoga-node16/pull/1#issuecomment-1161297244?

To me it seems like I did the same - I cannot spot the subtle difference 👀 😵

node_modules/@graphql-yoga/common/index.d.mts:4:15 - error TS7016: Could not find a declaration file for module '@envelop/core'. '/users/laurinquast/projects/yoga-node16/node_modules/@envelop/core/index.mjs' implicitly has an 'any' type.
  If the '@envelop/core' package actually exposes this module, try adding a new declaration (.d.ts) file containing `declare module '@envelop/core';`

4 export * from '@envelop/core';

node_modules/@graphql-yoga/subscription/createPubSub.d.ts:1:26 - error TS1471: Module '@repeaterjs/repeater' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import { Repeater } from '@repeaterjs/repeater';
                           ~~~~~~~~~~~~~~~~~~~~~~

node_modules/@graphql-yoga/subscription/operator/filter.d.ts:1:26 - error TS1471: Module '@repeaterjs/repeater' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import { Repeater } from '@repeaterjs/repeater';
                           ~~~~~~~~~~~~~~~~~~~~~~

node_modules/@graphql-yoga/subscription/operator/map.d.ts:1:26 - error TS1471: Module '@repeaterjs/repeater' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import { Repeater } from '@repeaterjs/repeater';

yes we definitely also have to solve this within envelop. Good think once we solved it for yoga (within bob, it should be straight forward to also introduce this on envelop) - but for repeaterjs I really don't see what you did differently than me.. 😕

@Duncan3142
Copy link
Author

Duncan3142 commented Jun 21, 2022

I think the problem is in '@graphql-yoga/subscription/index.d.mts'

You've created the correct .d.mts files, but you're referencing the d.ts files

So, instead of the following

export { createPubSub, PubSub } from './createPubSub.js';
export type { PubSubEventTarget, PubSubEvent } from './createPubSub.js';
export { map } from './operator/map.js';
export { filter } from './operator/filter.js';
export { pipe } from './utils/pipe.js';

we want

export { createPubSub, PubSub } from './createPubSub.mjs';
export type { PubSubEventTarget, PubSubEvent } from './createPubSub.mjs';
export { map } from './operator/map.mjs';
export { filter } from './operator/filter.mjs';
export { pipe } from './utils/pipe.mjs';

Note the different file extensions. This may be a problem in other files too, but it will be a variation on the same.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 21, 2022

@Duncan3142 Okay, so that means there is no way around shipping duplicated type definitions. 😢 But I guess this is fine for the transition period. Thank you so much for your research. I will keep you updated about the progress!

@Duncan3142
Copy link
Author

No worries, my pleasure. I think it probably is a necessity to have two set of declarations files. In this case, I think it's kinda cosmetic, so I can understand why it feels a bit cumbersome. However, if there was a use of dynamic imports, then they would have different semantics between commonjs and ESM, so having two sets is the most correct approach I think

@Duncan3142
Copy link
Author

Duncan3142 commented Jun 21, 2022

@n1ru4l There is one more thing I noticed in the code for your PR which would result in divergent behaviour, though my previous comments still apply. The package.json in the root of your forked repo is missing "type": "module"

This means typescript will always treat .ts files as commonjs files, so imports in those files will always transpile to 'require'. This would break the consumption of '@repeaterjs/repeater'

If the "type" field is missing or set to "commonjs", typescript files need to have the .mts extension to be treated as ESM

The following links go into detail:

Typescript 4.7 File Extensions
Typescript 4.7 Package Imports/Exports
Node package docs

@n1ru4l n1ru4l assigned n1ru4l and unassigned ardatan Jun 27, 2022
@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 27, 2022

@Duncan3142 I have good news, with all these canary releases, the yarn build command succeeds: https://github.com/Duncan3142/yoga-node16/pull/1/commits/3439bfdbc15e167ce9001bcd32ad94deecf93e8d

You can already start using the canaries and resolutions today.

We are missing a few reviews on kamilkisiela/bob#55, afterwards, we will ship a new minor version for all our packages!


PRs for introducing a proper exports map:

envelop: n1ru4l/envelop#1426
graphql-yoga: #1307
graphql-tools: ardatan/graphql-tools#4539

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 27, 2022

@Duncan3142 This is now fixed in this release #1303 😇

@graphql-yoga/common@2.9.0

@Duncan3142
Copy link
Author

@n1ru4l Sweet, that's fantastic. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants