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

refactor(cloudflare): make node:util/types a nodejs_compat module #329

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

anonrig
Copy link
Contributor

@anonrig anonrig commented Oct 11, 2024

All methods in util/types are implemented. We can remove the hybrid module status from node:util/types

@anonrig anonrig requested review from a team and pi0 as code owners October 11, 2024 20:18
@anonrig anonrig requested a review from jasnell October 11, 2024 20:18
@anonrig anonrig force-pushed the yagiz/remove-hybrid-module-util-types branch 6 times, most recently from 8809b04 to 4bc9c29 Compare October 11, 2024 20:28
@pi0 pi0 changed the title refactor(node/util/types): make util/types a nodejs_compat module refactor(cloudflare): make node:util/types a nodejs_compat module Oct 11, 2024
@vicb
Copy link
Contributor

vicb commented Oct 12, 2024

What workerd version this PR depends on?

We can not pull this code in wrangler before wrangler pulls the required version of workerd.

More generally I think we should use whatever workerd version is bundled with wrangler for the tests. Having a different version is a recipe for troubles.

@anonrig that's probably something we need to discuss internally.

@pi0
Copy link
Member

pi0 commented Oct 12, 2024

I think we discussed about it to split wrangler preset/versioning so we can test and upgrade unenv according to the latest workerd release.

@vicb
Copy link
Contributor

vicb commented Oct 12, 2024

I think we discussed about it to split wrangler preset/versioning so we can test and upgrade unenv according to the latest workerd release.

Are you referring to #262?

I think the problem we need to solve for wrangler is how to make sure unenv is compatible/tested with the version version of workerd it uses.

I'm unsure if creating a wrangler preset is the best way to solve this - hopefully this repo is never aware about wrangler. What I mean is that unenv patches should depend on workerd version, not wrangler version. When we have a way to do that then wrangler will pick the unenv version matching the workerd version it bundles.

@pi0
Copy link
Member

pi0 commented Oct 12, 2024

  • yes issue you mentioned is related -- we talked further in meeting about separating fully + npm tag for workerd
  • in unenv, we test against latest published workerd on npm
  • idea is that, workerd has a tag about the production version so wrangler tests against that
  • I think we probably should discuss it off this PR

@anonrig anonrig force-pushed the yagiz/remove-hybrid-module-util-types branch from 4bc9c29 to 811a7b6 Compare October 14, 2024 13:15
@anonrig
Copy link
Contributor Author

anonrig commented Oct 14, 2024

What workerd version this PR depends on?

This change was released almost 4 months ago - cloudflare/workerd@799dfbe. The change is first introduced in v1.20240614.0. @vicb

@anonrig
Copy link
Contributor Author

anonrig commented Oct 14, 2024

More generally I think we should use whatever workerd version is bundled with wrangler for the tests. Having a different version is a recipe for troubles.

@vicb I agree. In the long term, we should match those 2 version. Right now, wrangler main and unenv main has different workerd version.

@vicb
Copy link
Contributor

vicb commented Oct 14, 2024

More generally I think we should use whatever workerd version is bundled with wrangler for the tests. Having a different version is a recipe for troubles.

@vicb I agree. In the long term, we should match those 2 version. Right now, wrangler main and unenv main has different workerd version.

Maybe we should have 2 tags for workerd: "prod" and "wrangler" and test both versions.

@anonrig anonrig requested a review from pi0 October 14, 2024 13:44
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thank you ❤️

Feel free to merge when/if prod version of workerd shipped this

@vicb vicb merged commit 4685643 into main Oct 15, 2024
2 checks passed
@vicb vicb deleted the yagiz/remove-hybrid-module-util-types branch October 15, 2024 08:39
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.

4 participants