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: allow requireing alias modules #6932

Merged
merged 7 commits into from
Oct 9, 2024
Merged

fix: allow requireing alias modules #6932

merged 7 commits into from
Oct 9, 2024

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Oct 9, 2024

What this PR solves / how to test

Fixes #6896

Author has addressed the following

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

@vicb vicb requested a review from a team as a code owner October 9, 2024 07:33
Copy link

changeset-bot bot commented Oct 9, 2024

🦋 Changeset detected

Latest commit: 030bfe0

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

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers 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 marked this pull request as draft October 9, 2024 07:33
Copy link
Contributor

github-actions bot commented Oct 9, 2024

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/11253830172/npm-package-wrangler-6932

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-wrangler-6932 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-create-cloudflare-6932 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-cloudflare-kv-asset-handler-6932
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-miniflare-6932
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-cloudflare-pages-shared-6932
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-cloudflare-vitest-pool-workers-6932
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-cloudflare-workers-editor-shared-6932
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11253830172/npm-package-cloudflare-workers-shared-6932

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240925.1
workerd 1.20240925.0 1.20240925.0
workerd --version 1.20240925.0 2024-09-25

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@vicb vicb added the e2e Run e2e tests on a PR label Oct 9, 2024
@vicb vicb marked this pull request as ready for review October 9, 2024 09:13
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

A couple of queries...

@vicb
Copy link
Contributor Author

vicb commented Oct 9, 2024

/cc @pi0

@pi0
Copy link

pi0 commented Oct 9, 2024

It is a nice approach. I would suggest:

  1. See if we can directly make unenv npm compat exports more compatible. it is likely we can do this and avoid the overhead and risks of interop at esbuild level
  2. If we need interop, i would suggest to use something like this it is much safer. cjs interops can be really tricky.

@vicb
Copy link
Contributor Author

vicb commented Oct 9, 2024

Thanks for super valuable feedback @pi0!

I was hoping that given the restricted scope of this (/unenv/runtime/{npm,mock}) a simpler solution would be enough.

  • If we need interop, i would suggest to use something like this it is much safer. cjs interops can be really tricky.

But if you think this is not safe, using interopDefault should solve all the problems if I understand correctly?

  • See if we can directly make unenv npm compat exports more compatible. it is likely we can do this and avoid the overhead and risks of interop at esbuild level

One "issue" is that the unenv update in wrangler is paused for now because of a different issue. To make the change in unenv, we would need to update unenv and have a release without the perf_hook latest change. Might take longer than just releasing the change here (that we can revert later if no more needed).

@pi0
Copy link

pi0 commented Oct 9, 2024

Feel free to open a PR in unenv to revert that. (i think a simply uncomment in preset would solve it)

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Let's merge this for now and iterate on the concerns that @pi0 raised.

@vicb
Copy link
Contributor Author

vicb commented Oct 9, 2024

Let's merge this for now and iterate on the concerns that @pi0 raised.

I marked the convos as resolved (not sure if it was actually required for that repo).
But we should take them into account to iterate.

Also one thing I tried is that use the .cjs instead of of the .mjs as a simpler fix.

However const f = require('cross-fetch') would not have the named exports (i.e. f.Headers === undefined)

@vicb vicb merged commit 4c6aad0 into main Oct 9, 2024
22 checks passed
@vicb vicb deleted the require/alias branch October 9, 2024 13:21
@vicb vicb restored the require/alias branch October 9, 2024 13:21
@vicb vicb deleted the require/alias branch October 9, 2024 13:21
@workers-devprod workers-devprod mentioned this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Uncaught TypeError: utils.inherits is not a function
3 participants