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.

Sign up for GitHub

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

Jump to bottom

Add support for node:* imports with the nodejs_compat compatibility flag #2539

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Add support for node:* imports with the nodejs_compat compatibility flag #2539

merged 3 commits into from
Feb 27, 2023

Conversation

GregBrimble
Copy link
Member

@GregBrimble GregBrimble commented Jan 12, 2023 •
edited by JacobMGEvans
Loading

What this PR solves / how to test:

This PR does two things:

  1. Renames the --node-compat CLI/config option internally to legacyNodeCompat. Nothing changes externally. You still invoke it with --node-compat or node_compat. We maybe want to think about moving this to experimental as we stabilize thedev() API, but that discussion can happen elsewhere.
  2. Marks any imports prefixed with node: as external when bundling with the nodejs_compat compatibility flag.

Associated docs issues/PR:

Author has included the following, where applicable:

  • Tests (need a test to check the publish --dry-run and pages functions build output)
  • Changeset

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Fixes # [insert issue number].

@changeset-bot
Copy link

changeset-bot bot commented Jan 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 2e81412

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2023 •
edited
Loading

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4283766141/npm-package-wrangler-2539

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/2539/npm-package-wrangler-2539

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4283766141/npm-package-wrangler-2539 dev path/to/script.js
Additional artifacts: 
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4283766141/npm-package-cloudflare-pages-shared-2539

Note that these links will no longer work once the GitHub Actions artifact expires.

@codecov
Copy link

codecov bot commented Jan 12, 2023 •
edited
Loading

Codecov Report

Merging #2539 (2e81412) into main (f7d49eb) will increase coverage by 0.05%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2539      +/-   ##
==========================================
+ Coverage   73.97%   74.02%   +0.05%     
==========================================
  Files         166      166              
  Lines       10163    10191      +28     
  Branches     2705     2722      +17     
==========================================
+ Hits         7518     7544      +26     
- Misses       2645     2647       +2
Impacted Files Coverage Δ
packages/wrangler/src/dev/start-server.ts 66.02% <ø> (ø)
packages/wrangler/src/pages/buildFunctions.ts 92.30% <ø> (ø)
packages/wrangler/src/pages/dev.ts 18.69% <0.00%> (-0.25%) ⬇️
...ckages/wrangler/src/pages/functions/buildPlugin.ts 20.00% <ø> (ø)
...ckages/wrangler/src/pages/functions/buildWorker.ts 57.69% <ø> (ø)
packages/wrangler/src/paths.ts 100.00% <ø> (ø)
packages/wrangler/src/publish/index.ts 94.87% <ø> (ø)
packages/wrangler/src/api/dev.ts 60.21% <50.00%> (ø)
packages/wrangler/src/dev/validate-dev-props.ts 88.23% <50.00%> (-11.77%) ⬇️
packages/wrangler/src/pages/build.ts 84.90% <75.00%> (-1.10%) ⬇️
... and 7 more

@GregBrimble GregBrimble force-pushed the node-compat-take-2 branch 4 times, most recently from d9742f2 to 8f12780 Compare January 12, 2023 17:30
@GregBrimble GregBrimble marked this pull request as ready for review January 12, 2023 17:31
@GregBrimble GregBrimble requested review from a team as code owners January 12, 2023 17:31
@GregBrimble
Copy link
Member Author

GregBrimble commented Jan 12, 2023 •
edited
Loading

We need to add support for this compatibility flag in Miniflare (cloudflare/miniflare#473) but once the workerd PR is merged and released, this'll be available with --experimental-local.

JacobMGEvans
JacobMGEvans previously requested changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

Could we get tests for preventing users from running the legacy flag and the runtime flag together?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Jan 17, 2023 •
edited
Loading

There are checks for exclusivitity throughout?

Correct, I would like to see tests for that as a proof/coverage of that check functionality.

Edit: I apologize, my wording was ambiguous.

@danbars
Copy link

danbars commented Jan 18, 2023

What is the status of this PR?
Will it enable the use of async_hooks in worker scripts?

Currently I get this error when trying to use it:
The package "async_hooks" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "platform: 'node'" to do that, which will remove this error.

Is this PR related / will enable this workerd PR: cloudflare/workerd#208 ?

@GregBrimble
Copy link
Member Author

@danbars , still a work-in-progress! Please bear with us.

@danbars
Copy link

danbars commented Feb 22, 2023

Hi @GregBrimble, sorry for nagging, I simply have a module that is pending on this PR - using async_hooks from node.
I'll be happy to know what is the expected timeline for this.
If this is a few weeks I guess I'll wait. Otherwise I'll probably develop an alternative solution that does not have this dependency.

@GregBrimble GregBrimble force-pushed the node-compat-take-2 branch 3 times, most recently from 1b3943e to bb6e459 Compare February 27, 2023 14:23
@GregBrimble
Copy link
Member Author

@JacobMGEvans: rebased and added a couple of tests for that now:

@GregBrimble GregBrimble force-pushed the node-compat-take-2 branch 2 times, most recently from 52cb6d3 to 97b4201 Compare February 27, 2023 15:02
JacobMGEvans
JacobMGEvans approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

Looks great. I like the "conjunction" messaging and I appreciate the tests around trying to use them together.

@GregBrimble GregBrimble merged commit 3725086 into main Feb 27, 2023
@GregBrimble GregBrimble deleted the node-compat-take-2 branch February 27, 2023 15:16
@github-actions github-actions bot mentioned this pull request Feb 27, 2023
Merged
jspspike pushed a commit that referenced this pull request Feb 27, 2023
@GregBrimble @jspspike
Add support for node:* imports with the nodejs_compat compatibili…
5e605ae 
…ty flag (#2539)

* Fix capitalization of Node.js

* Internally rename `nodeCompat` to `legacyNodeCompat`

* Mark `node:*` imports as external when using the `nodejs_compat` compatibility flag
@irvinebroque
Copy link
Contributor

Should this work with Pages? See https://discord.com/channels/595317990191398933/1024343493251973190/1084637493787373630

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irvinebroque irvinebroque irvinebroque left review comments

@JacobMGEvans JacobMGEvans JacobMGEvans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GregBrimble @JacobMGEvans @danbars @irvinebroque