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

Process ESM .js modules in ./dist with Rollup, again. #6657

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 20, 2020

This partially reverts commit 627bd1b from PR #6656.

PR #6656 removed a function called prepareESM, which was no longer needed for the sake of running rollup-plugin-invariant, but which had another benefit: Rollup would replace imports/exports like

// Example exports from ./dist/index.js:
export * from './core'
export * from './react'

with their fully-expanded (and file-extensioned) equivalents:

...
export { makeVar } from './cache/inmemory/reactiveVars.js';
export { defaultDataIdFromObject } from './cache/inmemory/policies.js';
export { InMemoryCache } from './cache/inmemory/inMemoryCache.js';
export { ApolloClient } from './core/ApolloClient.js';
...

Although this may look like more code, it effectively saves the bundler/browser work by reducing indirection, and including the .js file extensions makes this code a little more browser- and Rollup-friendly (that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle @apollo/client with Rollup).

The expanded imports/exports are also closer to the ESM code shipped with @apollo/[email protected], so restoring this behavior helps with the goal of keeping the changes in #6656 backwards-compatible for v3.1.0.

Note that the CommonJS bundles used by Node.js are built before this step, so this expansion of imports does not affect how Node.js consumes CommonJS.

@benjamn benjamn added this to the Post 3.0 milestone Jul 20, 2020
@benjamn benjamn self-assigned this Jul 20, 2020
@benjamn benjamn force-pushed the run-rollup-with-preserveModules-again branch 3 times, most recently from 25c08a2 to e8604b3 Compare July 20, 2020 22:49
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Great catch @benjamn!

This partially reverts commit 627bd1b.

PR #6656 removed a function called `prepareESM`, which was no longer
needed for the sake of running `rollup-plugin-invariant`, but which had
another benefit: Rollup would replace imports/exports like

  // Example exports from ./dist/index.js:
  export * from './core'
  export * from './react'

with their fully-expanded (and file-extensioned) equivalents:

  ...
  export { makeVar } from './cache/inmemory/reactiveVars.js';
  export { defaultDataIdFromObject } from './cache/inmemory/policies.js';
  export { InMemoryCache } from './cache/inmemory/inMemoryCache.js';
  export { ApolloClient } from './core/ApolloClient.js';
  ...

Although this may look like more code, it's effectively what the
bundler/browser will have to do anyway, and including the `.js` file
extensions makes this code a little more browser- and Rollup-friendly
(that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle
@apollo/client with Rollup).

The expanded imports/exports are also closer to the ESM code shipped with
@apollo/[email protected], so restoring this behavior helps with the goal of
keeping the changes in #6656 backwards-compatible for v3.1.0.

Note that the CommonJS bundles used by Node.js are built before this step,
so this expansion of imports does not affect Node.js.
@benjamn benjamn force-pushed the run-rollup-with-preserveModules-again branch from e8604b3 to 1d57fc3 Compare July 21, 2020 14:26
@benjamn benjamn merged commit faa64b3 into master Jul 21, 2020
@benjamn benjamn deleted the run-rollup-with-preserveModules-again branch July 21, 2020 14:46
benjamn added a commit that referenced this pull request Jul 21, 2020
jimrandomh pushed a commit to jimrandomh/apollo-client that referenced this pull request Jul 22, 2020
…6657)

This partially reverts commit 627bd1b
from PR apollographql#6656.

PR apollographql#6656 removed a function called `prepareESM`, which was no longer
needed for the sake of running `rollup-plugin-invariant`, but which had
another benefit: Rollup would replace imports/exports like

  // Example exports from ./dist/index.js:
  export * from './core'
  export * from './react'

with their fully-expanded (and file-extensioned) equivalents:

  ...
  export { makeVar } from './cache/inmemory/reactiveVars.js';
  export { defaultDataIdFromObject } from './cache/inmemory/policies.js';
  export { InMemoryCache } from './cache/inmemory/inMemoryCache.js';
  export { ApolloClient } from './core/ApolloClient.js';
  ...

Although this may look like more code, it effectively saves the
bundler/browser work by reducing indirection, and including the .js file
extensions makes this code a little more browser- and Rollup-friendly
(that is, you shouldn't have to use @rollup/plugin-node-resolve to bundle
@apollo/client with Rollup).

The expanded imports/exports are also closer to the ESM code shipped with
@apollo/[email protected], so restoring this behavior helps with the goal of
keeping the changes in apollographql#6656 backwards-compatible for v3.1.0.

Note that the CommonJS bundles used by Node.js are built before this step,
so this expansion of imports does not affect Node.js.
jimrandomh pushed a commit to jimrandomh/apollo-client that referenced this pull request Jul 22, 2020
benjamn added a commit that referenced this pull request Jul 24, 2020
benjamn added a commit that referenced this pull request Jul 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 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.

2 participants