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

chore(unenv-preset): Add .cjs output for the preset #7721

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Jan 10, 2025

We do not need .cjs for the runtime files but wrangler requires the preset so we need to have a dist/index.cjs


  • Tests
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: not affected
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: No user facing change

@vicb vicb requested a review from a team as a code owner January 10, 2025 10:03
Copy link
Contributor

github-actions bot commented Jan 10, 2025

<style> pre { color: red; } </style>
npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12707700387/npmpackage-wrangler-7721

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/12707700387/npm-package-wrangler-7721

Copy link

changeset-bot bot commented Jan 10, 2025

🦋 Changeset detected

Latest commit: 160e9d9

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

This PR includes changesets to release 1 package
Name Type
@cloudflare/unenv-preset 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

@vicb vicb requested a review from petebacondarwin January 10, 2025 11:13
@petebacondarwin
Copy link
Contributor

petebacondarwin commented Jan 10, 2025

What do you mean "wrangler requires the preset"?
I don't see that happening in the code on main. E.g.

and also in the PR https://github.com/cloudflare/workers-sdk/pull/7720/files#diff-d5f05e17ffb161174d2bfea37bf23d39679290784e9aff28ac77bc06e3492459R3

@petebacondarwin
Copy link
Contributor

Or do you mean that if a library "requires" a Node.js module then our aliasing would result in the polyfilled module being required?

@vicb
Copy link
Contributor Author

vicb commented Jan 10, 2025

What do you mean "wrangler requires the preset"? I don't see that happening in the code on main. E.g.

and also in the PR https://github.com/cloudflare/workers-sdk/pull/7720/files#diff-d5f05e17ffb161174d2bfea37bf23d39679290784e9aff28ac77bc06e3492459R3

Wrangler is compiled to cjs (and unenv is external)

@petebacondarwin
Copy link
Contributor

OMG that seems unnecesssary. Perhaps we could just change that?

@vicb
Copy link
Contributor Author

vicb commented Jan 10, 2025

OMG that seems unnecesssary. Perhaps we could just change that?

The PR could still be useful if we change that - and I don't want to go down this rabbit hole today

@vicb
Copy link
Contributor Author

vicb commented Jan 10, 2025

Thanks @petebacondarwin

@vicb vicb merged commit 5c2c55a into main Jan 10, 2025
30 checks passed
@vicb vicb deleted the require-unenv branch January 10, 2025 12:54
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.

2 participants