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(builtin): remove unnecessary loader script #3495

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

kormide
Copy link
Collaborator

@kormide kormide commented Jun 27, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

This is a reincarnation of #3493, and related to #3277.

ESM scripts couldn't be used with nodejs_binary when --nobazel_run_linker was set. This is because the loader script is always a .cjs file and fails when it tries to load an mjs entrypoint.

What is the new behavior?

Remove the loader script as it no longer seems to be necessary.

Note that loading third-party dependencies in an esm script when the linker is disabled does not work as our require patch only applies to the cjs loader and not the mjs loader. However this change still provides incremental benefit as you can run an mjs script with no dependencies.

A solution that people can use to load third-party deps is to provide a custom loader using node's --loader option.

Does this PR introduce a breaking change?

  • Yes
  • No

Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

🌮

@kormide
Copy link
Collaborator Author

kormide commented Jun 28, 2022

The test failure is caused by this bug in rules_sass: bazelbuild/rules_sass#142

@kormide kormide force-pushed the remove-loader-script branch from 2de3b8a to 6960fdc Compare June 29, 2022 00:09
@kormide
Copy link
Collaborator Author

kormide commented Jun 29, 2022

Added a patch for rules_sass. The tests on buildkite are passing now.

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

Successfully merging this pull request may close these issues.

3 participants