Conversation
findWranglerConfig ran three find-up searches in sequence, so a wrangler.json anywhere up the tree was found before a wrangler.jsonc or wrangler.toml in the reference directory was ever tried. Walk one directory at a time instead, checking all three names per level. The same-directory preference (json, jsonc, toml) is unchanged. The cross-directory case was pinned the other way by a test cloudflare#7442 added around the existing behaviour; that expectation now names the nearer file, and two cases cover a three-level mix and a directory that has a config file's name.
🦋 Changeset detectedLatest commit: 3411150 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| /** | ||
| * Walk up from `referencePath` and return the first user configuration file found, | ||
| * checking every recognised file name in each directory before moving to its parent. | ||
| * | ||
| * Proximity wins over format: a `wrangler.jsonc` in the current directory is chosen | ||
| * over a `wrangler.json` in a parent. Searching for each file name all the way to the | ||
| * filesystem root before trying the next name would let an unrelated ancestor's config | ||
| * shadow the project's own. | ||
| */ | ||
| function findNearestUserConfig(referencePath: string): string | undefined { | ||
| for (const dir of walk.up(referencePath)) { | ||
| for (const name of USER_CONFIG_FILE_NAMES) { | ||
| const candidate = path.join(dir, name); | ||
| try { | ||
| if (statSync(candidate).isFile()) { | ||
| return candidate; | ||
| } | ||
| } catch { | ||
| // not present in this directory, try the next name | ||
| } | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Wouldn't this kind of a change / fix not be better served upstreamed to empathic/find? If it, afaict, seems to be performing a similar kind of file tree walking like empathic/find does but using statSync to check if it's a file?
Also with this new function is the find glob import even being used anymore?
There was a problem hiding this comment.
I believe find is still used on Line 135 of the branch, when findRedirectedWranglerConfig calls find.file(PATH_TO_DEPLOY_CONFIG, { cwd }), but the PR only replaced the three chained find.file calls inside findWranglerConfig; walk was added for the new function, and find stays for the redirect lookup
There was a problem hiding this comment.
agreed on the upstream to find, and appreciate the flag - I gave that a go!
There was a problem hiding this comment.
alrighty, the change should be in empathic 2.1!
Co-authored-by: Ben <4991309+NuroDev@users.noreply.github.com>
`statSync(p, { throwIfNoEntry: false })` is what cloudflare#12602 moved this package
to; the try/catch was copied from empathic, which supports Node 14 and
cannot use it. lukeed/empathic#14 adds `type: "file"` to `find.any`, which
is this function.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Tests (Windows, fixtures): entrypoints-rpc timed out on a dev-server fetch (UND_ERR_HEADERS_TIMEOUT). Wrangler E2E (macOS, shard 4/4): cancelled at the 30 minute job timeout while the other 11 shards passed, including shard 4 on Linux and Windows. The job that failed on the previous run passed on this one.
|
Question: would this not be a breaking change as per an older PR where a very similar implementation was rejected for this reason? #7799 (comment) |
4 participants
Fixes #15663
findWranglerConfigran three find-up searches one after the other, sowrangler.jsonwas searched all the way to the filesystem root beforewrangler.jsoncorwrangler.tomlin the reference directory was looked at. Awrangler.jsonin any ancestor directory therefore shadowed the project's own config, silently:wrangler deployin a directory holding a perfectly goodwrangler.jsoncprinted the ancestor's bindings and would have built the ancestor's Worker. The four-line reproduction and the story of how I hit it are on the issue.What changed
findNearestUserConfigwalks up from the reference path one directory at a time (empathic/walk, already a dependency) and checks all three file names in each directory before moving to its parent. The same-directory preference is unchanged:wrangler.json, thenwrangler.jsonc, thenwrangler.toml.find.filecalls it replaces.empathic'sfind.any(names)does the per-directory walk in fewer lines but matches withexistsSync, so a directory namedwrangler.jsonwould have won; there is a test for that shape now.findRedirectedWranglerConfigis untouched.The test that pinned the old behaviour
findWranglerConfig.test.tshadshould prefer the wrangler.%s over wrangler.%s > in different directories, which asserted that a rootwrangler.jsonbeatsfoo/wrangler.jsoncwhen searching from./foo. It came in with #7442 around code that was already there, and I could not find a discussion of the cross-directory case in that PR or elsewhere, so I have read it as a test that captured what the code did rather than a decision. That expectation now names the nearer file, and the describe block is renamed to say what it actually asserts (only when they are in the same directory). Two cases are added: a three-level json/toml/jsonc mix searched from each level, and a directory carrying a config file's name.If the format-over-proximity order was intentional, I would rather this became a docs PR saying so than land; nothing currently documents it either way.
Locally:
pnpm -F @cloudflare/workers-utils test1066/1066; wrangler'sentry-points,deploy/core,pages/deploysuites 166/166 against the rebuiltworkers-utils;oxfmtandoxlint --type-awareclean on both files. Control: with the source change stashed, the changed expectation and the new three-level case fail against the old implementation (4 failures), so the tests are asserting the behaviour rather than the fixture.