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

Revert normalizeNonRelativeId changes in config/resolveModuleIds.ts #9073

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Nov 16, 2021

This reverts commit 60e18e9 from PR #8396.

In theory, commit 60e18e9 should have made it easier to consume @apollo/client ESM modules in a browser environment, by rewriting non-relative imported module identifiers like

import { useState } from "react"

to

import { useState } from "react/index.js"

effectively bypassing react/package.json and its "main" field (which is not understood by browsers). Of course, the react package doesn't actually export ESM modules, and therefore can't be used natively in the browser anyway, so the rewriting doesn't help in this case.

Not helping would be one thing, but unfortunately this change introduces more problems for bundlers that expect to find a package.json file when importing non-relative dependencies. For example, webpack complains that react/index.js does not export functions such as useState (a fair criticism, since react/index.js is a CommonJS module).

Reviewing the commit message from 60e18e9, the only third-party package it names as benefitting from this extra resolution step is ts-invariant/process, which is a package we control and therefore can fix if necessary. My plan is to merge this PR into release-3.6 so we can do some additional testing of beta releases before backporting this change to v3.5.x (the main branch).

Here's a diff of import changes generated as a result of this PR, all of which seem reasonable to me: https://gist.github.com/benjamn/31c6ca0d61ca12bd42f08d3c6e6ef42b

@benjamn benjamn self-assigned this Nov 16, 2021
@benjamn benjamn added this to the v3.5.x patch releases milestone Nov 16, 2021
@benjamn
Copy link
Member Author

benjamn commented Nov 16, 2021

I can confirm these changes still allow @apollo/client@3.6.0-beta.0 to be consumed through an ESM-aware CDN:

await import("https://cdn.jsdelivr.net/npm/@apollo/client@3.6.0-beta.0/core/+esm")

Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

LGTM

@SimenB
Copy link
Contributor

SimenB commented Nov 18, 2021

I'm still getting an error from webpack after upgrading to 3.5.3

ERROR in ../../node_modules/@apollo/client/utilities/globals/fix-graphql.js 1:0-46
Module not found: Error: Can't resolve 'ts-invariant/process' in '/workspace/node_modules/@apollo/client/utilities/globals'
Did you mean 'index.js'?
BREAKING CHANGE: The request 'ts-invariant/process' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.
resolve 'ts-invariant/process' in '/workspace/node_modules/@apollo/client/utilities/globals'
  Parsed request is a module
  using description file: /workspace/node_modules/@apollo/client/utilities/globals/package.json (relative path: .)
    Field 'browser' doesn't contain a valid alias configuration
    resolve as module
      /workspace/node_modules/@apollo/client/utilities/globals/node_modules doesn't exist or is not a directory
      /workspace/node_modules/@apollo/client/utilities/node_modules doesn't exist or is not a directory
      looking for modules in /workspace/node_modules/@apollo/client/node_modules
        /workspace/node_modules/@apollo/client/node_modules/ts-invariant doesn't exist
      /workspace/node_modules/@apollo/node_modules doesn't exist or is not a directory
      /workspace/node_modules/node_modules doesn't exist or is not a directory
      looking for modules in /workspace/node_modules
        existing directory /workspace/node_modules/ts-invariant
          using description file: /workspace/node_modules/ts-invariant/package.json (relative path: .)
            using description file: /workspace/node_modules/ts-invariant/package.json (relative path: ./process)
              Field 'browser' doesn't contain a valid alias configuration
              /workspace/node_modules/ts-invariant/process is not a file
      /node_modules doesn't exist or is not a directory

@brainkim
Copy link
Contributor

@SimenB Also seeing this in our internal stuff. We’re on it!

benjamn added a commit that referenced this pull request Nov 18, 2021
From @apollo/client@3.5.0 until my recent PR #9073 (v3.5.3),
non-relative imported module identifiers like 'ts-invariant/process'
were rewritten during the build process (during 'npm run resolve') to
include a specific module with a file extension (not a directory),
producing in this example 'ts-invariant/process/index.js'.

That rewriting was the wrong choice in general (for example, it would
rewrite 'graphql' to 'graphql/index.mjs'), so PR #9073 disabled it for
all non-relative imports.

However, the specific replacement of 'ts-invariant/process' with
'ts-invariant/process/index.js' is still useful to prevent the strange
webpack 5-specific error

  The request 'ts-invariant/process' failed to resolve only because it was resolved as fully specified

which apparently happens because the
[resolve.fullySpecified](https://webpack.js.org/configuration/module/#resolvefullyspecified)
option is true by default now, and 'ts-invariant/process/package.json'
is ignored, effectively forbidding directory-style imports like
'ts-invariant/process' when the package.json of the enclosing package
has `"type": "module"` (which became true in Apollo Client v3.5, thanks
to #8396). App developers could configure resolve.fullySpecified
explicitly false, but we (the Apollo Client team) prefer a general
workaround.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants