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: add support for custom unenv resolve path #7522

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Dec 11, 2024

We are moving the unenv preset to this repo.

We will need to be able to test the version that is package with wrangler as well as the dev version. We can do that by passing the path to the dev version in the added env var.

/cc @pi0


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

@vicb vicb requested a review from a team as a code owner December 11, 2024 15:24
Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 37aeba8

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 requested a review from petebacondarwin December 11, 2024 15:25
Copy link
Contributor

github-actions bot commented Dec 11, 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/12370972522/npm-package-wrangler-7522

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-wrangler-7522 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-workers-bindings-extension-7522 -O ./cloudflare-workers-bindings-extension.0.0.0-v638d96862.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v638d96862.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-create-cloudflare-7522 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-kv-asset-handler-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-miniflare-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-pages-shared-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-vitest-pool-workers-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-workers-editor-shared-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-workers-shared-7522
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12370972522/npm-package-cloudflare-workflows-shared-7522

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.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

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

@vicb vicb marked this pull request as draft December 11, 2024 17:04
@vicb vicb requested a review from petebacondarwin December 16, 2024 10:27
@vicb vicb force-pushed the unenv-preset-path branch from 5a39e9e to 20bb308 Compare December 17, 2024 08:53
@vicb vicb added the e2e Run e2e tests on a PR label Dec 17, 2024
@vicb vicb marked this pull request as ready for review December 17, 2024 08:54
@vicb vicb requested a review from pi0 December 17, 2024 08:54
@pi0
Copy link
Contributor

pi0 commented Dec 17, 2024

LGTM! Small point: It is technically UNENV_RESOLVE_PATHS not specific to preset paths only 😄

@vicb vicb force-pushed the unenv-preset-path branch from 8a85559 to 3023c61 Compare December 17, 2024 09:28
@vicb vicb changed the title feat: add support for custom unenv preset path feat: add support for custom unenv resolve path Dec 17, 2024
@vicb vicb force-pushed the unenv-preset-path branch from 3023c61 to 2e013e1 Compare December 17, 2024 09:49

feat(wrangler): allow overriding the unenv preset paths

By default wrangler uses the unenv preset installed via package.json.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify this statement. Which package.json are we referring to here? Is it the user's one or the one inside the wrangler package?

Copy link
Contributor Author

@vicb vicb Dec 17, 2024

Choose a reason for hiding this comment

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

I updated the description, PTAL.


By default wrangler uses the unenv preset installed via package.json.

Setting `WRANGLER_UNENV_RESOLVE_PATHS` allow to use a local version of the preset.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this naming a bit confusing. I think that we are only using this path to resolve from imports like unenv/foo/bar to some absolute path, right? It doesn't affect other imports? Are we really just providing custom paths to the unenv package? Is there some other use for thes paths? If not, then I would expect this to be more like WRANGLER_UNENV_PACKAGE_PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paths are used when unenv resolve the absolute paths.

The module specifiers could be unenv/... or @cloudflare/unenv-preset/...

I like "RESOLVE_PATHS" because it matches require.resolve(id, { paths }) but happy to change if you have strong feeling on this one.

@vicb vicb merged commit 6403e41 into main Dec 17, 2024
31 checks passed
@vicb vicb deleted the unenv-preset-path branch December 17, 2024 12:08
penalosa added a commit that referenced this pull request Dec 18, 2024
CarmenPopoviciu pushed a commit that referenced this pull request Dec 18, 2024
* Revert "feat: add support for custom unenv resolve path (#7522)"

This reverts commit 6403e41.

* Create thick-ties-glow.md

* Revert "chore(wrangler): update unenv dependency version (#7541)"

This reverts commit ca9410a.

* fix lockfile
penalosa pushed a commit that referenced this pull request Jan 10, 2025
feat(wrangler): add support for custom unenv resolve path

Introduce `WRANGLER_UNENV_RESOLVE_PATHS` to
specify resolve paths for unenv.

Co-authored-by: Pete Bacon Darwin <[email protected]>
penalosa added a commit that referenced this pull request Jan 10, 2025
* Revert "feat: add support for custom unenv resolve path (#7522)"

This reverts commit 6403e41.

* Create thick-ties-glow.md

* Revert "chore(wrangler): update unenv dependency version (#7541)"

This reverts commit ca9410a.

* fix lockfile
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants