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.

Sign up for GitHub

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 export specification #9697

Closed
wants to merge 5 commits into from
Closed

Add export specification #9697

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ab4e49
Add export specification
tobiasdiez May 8, 2022
1ccdfce
Add main entry point
tobiasdiez May 8, 2022
7b2f3c3
Merge branch 'main' into patch-2
tobiasdiez Sep 8, 2022
6335636
Merge branch 'main' into patch-2
tobiasdiez Sep 8, 2022
3dca41f
Merge branch 'main' into patch-2
tobiasdiez Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,23 @@
"main": "./dist/main.cjs",
"module": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
".": {
"import": "./dist/index.js",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without "type": "module" set, .js is interpreted as CJS, not ESM. You need to use .mjs to refer to ESM in a CJS package (which is the default).

"require": "./dist/main.cjs"
},
"./core": {
"import": "./core/index.js",
"require": "./core/core.cjs"
},
"./link/error": {
"import": "./link/error/index.js",
"require": "./link/error/error.cjs"
},
"./utilities/policies/pagination": {
"import": "./utilities/policies/pagination.js"
}
},
Comment on lines 16 to +35
Copy link
Contributor

@rosskevin rosskevin Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be compliant, I read the typescript handbook as the following (remove commented lines, add in types, and note the wrong published module path has not dist). Also note I added in the link/core module:

  // -"main": "./main.cjs",
  // -"module": "./index.js",
  // -"types": "./index.d.ts",
  "exports": {
    ".": {
      "types": "./index.d.ts",
      "import": "./index.js",
      "require": "./main.cjs"
    },
    "./core": {
      "types": "./core/index.d.ts",
      "import": "./core/index.js",
      "require": "./core/core.cjs"
    },
    "./link/core": {
      "types": "./link/core/index.d.ts",
      "import": "./link/core/index.js",
      "require": "./link/core/error.cjs"
    },
    "./link/error": {
      "types": "./link/error/index.d.ts",
      "import": "./link/error/index.js",
      "require": "./link/error/error.cjs"
    },
    "./utilities/policies/pagination": {
      "types": "./utilities/policies/pagination.js",
      "import": "./utilities/policies/pagination.js"
    }
  },

I will note however, that while I think the above exports are correct according to the handbook, that I still receive errors when attempting to use option "moduleResolution": "nodenext", but it is fine when using the more typical node resolution. (I believe this to be a bug with typescript and export * from using nodenext resolution and I'm working on a test case for them).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the input. I believe though that the dist prefix is necessary, since that's where the files are in the distributed package. Feel free to add your other changes as github suggestions, then I will merge them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasdiez I have confirmed that dist in not in the published package. See https://unpkg.com/@apollo/client@3.6.6/dist or
tsconfig_react-lib_json_—_ts-esm-workspaces

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you are right and there is indeed no dist folder. But the dist path is akso in "main" and "module", strange...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These paths must be rewritten during the build process. If you yarn patch @apollo/client and open an editor, you can see the produced paths more easily.

"sideEffects": false,
"react-native": {
"./dist/cache/inmemory/fixPolyfills.js": "./dist/cache/inmemory/fixPolyfills.native.js"
Expand Down