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

fix(build): Reintroduce shims for subpackage entrypoints #8050

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

cpcallen
Copy link
Contributor

The basics

The details

Resolves

Proposed Changes

Reintroduce shims (dist/core.js, dist/blocks.js, dist/javascript.js, etc.) for submodule entrypoints in top level of package. Have them created by package_tasks.js directly, rather than being copied from scripts/package/.

Assumptions:

  • Such bundlers will completely ignore the exports declaration.
  • The bundles are intended to be used in a browser—or at least not in node.js—so the core entrypoint never needs to route to core-node.js. This is reasonable since there's little reason to bundle code for node.js, and node.js has supported the exports clause since at least v12, considerably older than any version of node.js we officially support.
  • It suffices to provide only a CJS entrypoint (because we can only provide CJS or ESM, not both. (We could in future switch to providing only an ESM entrypoint instead, though.)

Reason for Changes

The v11 beta breaks builds for external developers using browserify.

This change should solve issues encountered by users of bundlers that don't support exports at all (e.g. browserify) as well as ones that don't support it in certain circumstances (e.g., when using webpack's [resolve.alias] configuration option](https://webpack.js.org/configuration/resolve/#resolvealias) to alias 'blockly' to 'node_modules/blockly', as we formerly did in most plugins, which causes webpack to ignore blockly's package.json` entirely).

Documentation

We should mention in next beta release notes that even bundlers that do not support exports should continue to work.

Additional Information

This is in effect a partial rollback of PR #7822.

This is in effect a partial rollback of PR google#7822.

This should solve issues encountered by users of bunders that don't
support exports at all (e.g. browserify) as well as ones that don't
support it in certain circumstances (e.g., when using webpack's
resolve.alias configuration option to alias 'blockly' to
'node_modules/blockly', as we formerly did in most plugins, which
causes webpack to ignore blockly's package.json entirely).

Assumptions:
- Such bundlers will _completely_ ignore the exports declaration.
- The bundles are intended to be used in a browser—or at least not
  in node.js—so the core entrypoint never needs to route to
  core-node.js.  This is reasonable since there's little reason to
  bundle code for node.js, and node.js has supported the exports
  clause since at least v12, consideably older than any version of
  node.js we officially support.
- It suffices to provide only a CJS entrypoint (because we can only
  provide CJS or ESM, not both.  (We could in future switch to
  providing only an ESM entrypoint instead, though.)
@cpcallen cpcallen requested a review from BeksOmega April 24, 2024 17:28
@cpcallen cpcallen requested a review from a team as a code owner April 24, 2024 17:28
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Apr 24, 2024
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

This looks good to me! Pulled it down and tested that the generated shims look correct.

Curious: does this support people using typescript + browserify? I think you mentioned something briefly about shimming the .d.ts but I don't think we fully discussed it at the team meeting.

@cpcallen
Copy link
Contributor Author

Curious: does this support people using typescript + browserify? I think you mentioned something briefly about shimming the .d.ts but I don't think we fully discussed it at the team meeting.

Very conveniently there is already a .d.ts file for each of the new .js wrappers, so typescript type checking should work correctly, both for TSC and in the browser (even if the latter does obey the exports directive, which specifies the path to use.).

@cpcallen cpcallen merged commit a062ab8 into google:rc/v11.0.0 Apr 24, 2024
10 checks passed
@cpcallen cpcallen deleted the feat/shims branch April 24, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants