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

Fix relative project root #7285

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix relative project root #7285

wants to merge 5 commits into from
+203 −59

Conversation

penalosa
Copy link
Contributor

#6971 introduced a small regression in the getEntry() function that meant that Entry.directory was always process.cwd() rather than relative to the found wrangler.toml file. This PR fixes the regression & adds a regression test that fails on main but passes prior to #6971.

  • 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:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bugfix

@penalosa penalosa requested a review from a team as a code owner November 18, 2024 17:38
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 18, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: c1ceac2

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

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@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

@penalosa penalosa added the regression Break in existing functionality as a result of a recent change label Nov 18, 2024
CarmenPopoviciu
CarmenPopoviciu reviewed Nov 18, 2024
View reviewed changes
packages/wrangler/src/__tests__/get-entry.test.ts
JSON.stringify(entry).replaceAll(process.cwd(), "/tmp/dir")
);
}
describe("getEntry()", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏 🙇 ♥️

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 18, 2024 •
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/11917170436/npm-package-wrangler-7285

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

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

Or you can use npx with this latest build directly:

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

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.20241106.0
workerd 1.20241106.1 1.20241106.1
workerd --version 1.20241106.1 2024-11-06

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

@penalosa penalosa force-pushed the penalosa/relative-dot-wrangler branch from 865d102 to 577f4b9 Compare November 18, 2024 18:18
petebacondarwin
petebacondarwin reviewed Nov 18, 2024
View reviewed changes
packages/wrangler/src/deployment-bundle/entry.ts Outdated
@@ -53,7 +53,9 @@ export async function getEntry(
const directory = process.cwd();
const entryPoint = config.site?.["entry-point"];

let paths: { absolutePath: string; relativePath: string } | undefined;
let paths:
| { absolutePath: string; relativePath: string; directory?: string }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| { absolutePath: string; relativePath: string; directory?: string }
| { absolutePath: string; relativePath: string; projectRoot?: string }

packages/wrangler/src/deployment-bundle/resolve-entry.ts Outdated
@@ -16,11 +16,12 @@ export function resolveEntryWithMain(
): {
absolutePath: string;
relativePath: string;
directory: string;
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 call this projectRoot? Will simplify code elsewhere and is more descriptive than directory which could be a directory of anything.

packages/wrangler/src/__tests__/get-entry.test.ts Outdated
describe("getEntry()", () => {
runInTempDir();
mockConsoleMethods();
it.each([
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that you added these tests, but if I am honest, using it.each() here actually obscures what the tests are doing and doesn't save much boilerplate compared to writing out a bunch of separate it clauses, given how simple the test code is.

@penalosa
Copy link
Contributor Author

@petebacondarwin I've renamed directory to projectRoot in the Entry type & SDW types, which should clarify things across the codebase. This is a breaking change to the SDW interface, which is fine because that's currently exposed as an experimental API

@penalosa penalosa requested a review from a team as a code owner November 19, 2024 14:28
petebacondarwin
petebacondarwin approved these changes Nov 19, 2024
View reviewed changes
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.

Cripes that rename had more blast radius than I thought!
Thanks.

A few unused imports to remove?

@GregBrimble
Copy link
Member

GregBrimble commented Nov 19, 2024 •
edited
Loading

Haven't tested, but this maybe fixes [break autolink] #5481?

@penalosa
Copy link
Contributor Author

#5481

It shouldn't I don't think—that looks older than this regression? Taking a look

@penalosa
Copy link
Contributor Author

@GregBrimble it looks like it might be a workspaces thing? #5481 (comment)

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

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@CarmenPopoviciu CarmenPopoviciu Awaiting requested review from CarmenPopoviciu

Assignees
No one assigned
Labels
regression Break in existing functionality as a result of a recent change
Projects
workers-sdk
Status: Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@penalosa @GregBrimble @petebacondarwin @CarmenPopoviciu