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

feat(builtin): add basic ESM support #3294

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

Toxicable
Copy link

@Toxicable Toxicable commented Jan 27, 2022

PR Checklist

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

PR Type

  • Feature (please, look at the "Scope of the project" section in the README.md file)

What is the current behavior?

Issue Number: #3277

What is the new behavior?

Adds ESM support in for nodejs_binary, jasmine_node_test and ts_project

Does this PR introduce a breaking change?

  • No - Only changes are internal API's

@Toxicable Toxicable force-pushed the esm-support branch 2 times, most recently from 7001abf to 5c7d4d2 Compare January 27, 2022 22:21
@Toxicable
Copy link
Author

Things to note:

  • requires typescript > 4.5
  • requires jasmine > 3.7
  • Editor support requires a manual file association for *.mts -> typescript
  • Imports are required to end with .mjs and there's not currently editor support (ticket likely exists needs to be found or created)
  • Typescript reference https://www.typescriptlang.org/docs/handbook/esm-node.html

@Toxicable Toxicable force-pushed the esm-support branch 4 times, most recently from fab02f1 to 8364d3c Compare January 27, 2022 22:44
@Toxicable
Copy link
Author

I've extracted ts_project support into a seperate MR #3298

@Toxicable Toxicable force-pushed the esm-support branch 4 times, most recently from 4aac85b to adb1158 Compare January 28, 2022 01:25
@Toxicable
Copy link
Author

CI looks mostly green now
Aside from this failure, which looks like it's unrelated and only on windows

ERROR: C:/b/bk-windows-zzbr/bazel/rules-nodejs-nodejs/internal/node/test/chdir/BUILD.bazel:20:16: output 'internal/node/test/chdir/package.json' was not created

Copy link
Contributor

@devversion devversion 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 great to me.

@@ -1,2 +1,3 @@
tag-version-prefix=""
message="chore(release): %s"
registry=https://registry.npmjs.org
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentionally part of this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, this change isn't related to this PR, but I needed it so that yarn wouldn't try pull packages from another registry that I have configured.
I don't think anyone else would have this issue, but it wouldn't cause an issue leaving it in as well.
Happy to remove if you think so

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem with me, just asked since it seemed unrelated 😄

packages/jasmine/jasmine_runner.js Outdated Show resolved Hide resolved
@alexeagle alexeagle merged commit b4b2c74 into bazel-contrib:stable Mar 30, 2022
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