Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@petebacondarwin
petebacondarwin committed Dec 23, 2024
1 parent 7747092 commit 0eea310
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 60 deletions.
55 changes: 27 additions & 28 deletions packages/wrangler/src/__tests__/config/findWranglerConfig.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,16 @@ describe("config findWranglerConfig()", () => {
"out": "",
"warn": "▲ [WARNING] Using redirected Wrangler configuration.
Redirected config path: \\"dist/wrangler.json\\"
Deploy config path: \\".wrangler/deploy/config.json\\"
Original config path: \\"<no user config found>\\"
Configuration being used: \\"dist/wrangler.json\\"
Original user's configuration: \\"<no user config found>\\"
Deploy configuration file: \\".wrangler/deploy/config.json\\"
▲ [WARNING] Using redirected Wrangler configuration.
Redirected config path: \\"dist/wrangler.json\\"
Deploy config path: \\".wrangler/deploy/config.json\\"
Original config path: \\"<no user config found>\\"
Configuration being used: \\"dist/wrangler.json\\"
Original user's configuration: \\"<no user config found>\\"
Deploy configuration file: \\".wrangler/deploy/config.json\\"
",
}
Expand Down Expand Up @@ -144,16 +144,16 @@ describe("config findWranglerConfig()", () => {
"out": "",
"warn": "▲ [WARNING] Using redirected Wrangler configuration.
Redirected config path: \\"dist/wrangler.json\\"
Deploy config path: \\".wrangler/deploy/config.json\\"
Original config path: \\"wrangler.toml\\"
Configuration being used: \\"dist/wrangler.json\\"
Original user's configuration: \\"wrangler.toml\\"
Deploy configuration file: \\".wrangler/deploy/config.json\\"
▲ [WARNING] Using redirected Wrangler configuration.
Redirected config path: \\"dist/wrangler.json\\"
Deploy config path: \\".wrangler/deploy/config.json\\"
Original config path: \\"wrangler.toml\\"
Configuration being used: \\"dist/wrangler.json\\"
Original user's configuration: \\"wrangler.toml\\"
Deploy configuration file: \\".wrangler/deploy/config.json\\"
",
}
Expand All @@ -172,17 +172,16 @@ describe("config findWranglerConfig()", () => {
error = e;
}

expect(normalizeString(`${error}`).replace(process.cwd(), "<cwd>"))
.toMatchInlineSnapshot(`
"Error: Failed to load the deploy config at .wrangler/deploy/config.json
X [ERROR] InvalidSymbol
expect(normalizeString(`${error}`)).toMatchInlineSnapshot(`
"Error: Failed to parse the deploy configuration file at .wrangler/deploy/config.json
X [ERROR] InvalidSymbol
<cwd>/.wrangler/deploy/config.json:1:0:
[37m 1 │ [32mINVALID[37m JSON
╵ [32m~~~~~~~[0m
<cwd>/.wrangler/deploy/config.json:1:0:
[37m 1 │ [32mINVALID[37m JSON
╵ [32m~~~~~~~[0m
"
`);
"
`);
expect(std).toEqual(NO_LOGS);
});

Expand All @@ -199,7 +198,7 @@ describe("config findWranglerConfig()", () => {
}

expect(normalizeString(`${error}`)).toMatchInlineSnapshot(`
"Error: A redirect config was found at \\".wrangler/deploy/config.json\\".
"Error: A deploy configuration file was found at \\".wrangler/deploy/config.json\\".
But this is not valid - the required \\"configPath\\" property was not found.
Instead this file contains:
\`\`\`
Expand All @@ -222,8 +221,8 @@ describe("config findWranglerConfig()", () => {
}

expect(normalizeString(`${error}`)).toMatchInlineSnapshot(`
"Error: There is a redirect configuration at \\".wrangler/deploy/config.json\\".
But the config path it points to, \\".wrangler/deploy/missing/wrangler.json\\", does not exist."
"Error: There is a deploy configuration at \\".wrangler/deploy/config.json\\".
But the redirected configuration path it points to, \\".wrangler/deploy/missing/wrangler.json\\", does not exist."
`);
expect(std).toEqual(NO_LOGS);
});
Expand All @@ -247,8 +246,8 @@ describe("config findWranglerConfig()", () => {
}

expect(normalizeString(`${error}`)).toMatchInlineSnapshot(`
"Error: Found both a user config file at \\"foo/wrangler.toml\\"
and a redirect config file at \\"foo/bar/.wrangler/deploy/config.json\\".
"Error: Found both a user configuration file at \\"foo/wrangler.toml\\"
and a deploy configuration file at \\"foo/bar/.wrangler/deploy/config.json\\".
But these do not share the same base path so it is not clear which should be used."
`);
expect(std).toEqual(NO_LOGS);
Expand All @@ -261,8 +260,8 @@ describe("config findWranglerConfig()", () => {
}

expect(normalizeString(`${error}`)).toMatchInlineSnapshot(`
"Error: Found both a user config file at \\"bar/foo/wrangler.toml\\"
and a redirect config file at \\"bar/.wrangler/deploy/config.json\\".
"Error: Found both a user configuration file at \\"bar/foo/wrangler.toml\\"
and a deploy configuration file at \\"bar/.wrangler/deploy/config.json\\".
But these do not share the same base path so it is not clear which should be used."
`);
expect(std).toEqual(NO_LOGS);
Expand Down
21 changes: 5 additions & 16 deletions packages/wrangler/src/__tests__/d1/migrate.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ describe("migrate", () => {
// If we get to the point where we are checking for migrations then we have not been asked to log in.
await expect(
runWrangler("d1 migrations apply DATABASE")
).rejects.toThrowError(
`No migrations present at ${cwd().replaceAll("\\", "/")}/migrations.`
);
).rejects.toThrowError(`No migrations present at <cwd>/migrations.`);
});

it("should try to read D1 config from wrangler.toml", async () => {
Expand All @@ -68,9 +66,7 @@ describe("migrate", () => {
// If we get to the point where we are checking for migrations then we have not checked wrangler.toml.
await expect(
runWrangler("d1 migrations apply DATABASE")
).rejects.toThrowError(
`No migrations present at ${cwd().replaceAll("\\", "/")}/migrations.`
);
).rejects.toThrowError(`No migrations present at <cwd>/migrations.`);
});

it("should reject the use of --preview with --local", async () => {
Expand Down Expand Up @@ -221,9 +217,7 @@ Your database may not be available to serve requests during the migration, conti
// If we get to the point where we are checking for migrations then we have not been asked to log in.
await expect(
runWrangler("d1 migrations list --local DATABASE")
).rejects.toThrowError(
`No migrations present at ${cwd().replaceAll("\\", "/")}/migrations.`
);
).rejects.toThrowError(`No migrations present at <cwd>/migrations.`);
});

it("should use the custom migrations folder when provided", async () => {
Expand All @@ -241,10 +235,7 @@ Your database may not be available to serve requests during the migration, conti
await expect(
runWrangler("d1 migrations list --local DATABASE")
).rejects.toThrowError(
`No migrations present at ${cwd().replaceAll(
"\\",
"/"
)}/my-migrations-go-here.`
`No migrations present at <cwd>/my-migrations-go-here.`
);
});

Expand Down Expand Up @@ -276,9 +267,7 @@ Your database may not be available to serve requests during the migration, conti
// If we get to the point where we are checking for migrations then we have not checked wrangler.toml.
await expect(
runWrangler("d1 migrations list DATABASE")
).rejects.toThrowError(
`No migrations present at ${cwd().replaceAll("\\", "/")}/migrations.`
);
).rejects.toThrowError(`No migrations present at <cwd>/migrations.`);
});
});
});
9 changes: 8 additions & 1 deletion packages/wrangler/src/__tests__/helpers/normalize.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export function normalizeString(input: string): string {
return normalizeErrorMarkers(
replaceByte(
stripTrailingWhitespace(
normalizeSlashes(normalizeTempDirs(stripTimings(input)))
normalizeSlashes(normalizeCwd(normalizeTempDirs(stripTimings(input))))
)
)
);
Expand All @@ -29,6 +29,13 @@ function normalizeSlashes(str: string): string {
return str.replace(/\\/g, "/");
}

/**
* Replace any use of the current working directory with `<cwd>` to avoid cross OS issues.
*/
function normalizeCwd(str: string): string {
return str.replaceAll(process.cwd(), "<cwd>");
}

/**
* Strip "timing data" out of the `stdout` string, since this is not always deterministic.
*
Expand Down
22 changes: 11 additions & 11 deletions packages/wrangler/src/config/config-helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export function resolveWranglerConfigPath(
}

/**
* Find the wrangler config file by searching up the file-system
* Find the wrangler configuration file by searching up the file-system
* from the current working directory.
*/
export function findWranglerConfig(
Expand All @@ -64,7 +64,7 @@ export function findWranglerConfig(
}

/**
* Check whether there is a config file that indicates that we should redirect the user configuration.
* Check whether there is a configuration file that indicates that we should redirect the user configuration.
* @param cwd
* @param userConfigPath
* @returns
Expand Down Expand Up @@ -92,14 +92,14 @@ function findRedirectedWranglerConfig(
} catch (e) {
throw new UserError(
dedent`
Failed to load the deploy config at ${path.relative(".", deployConfigPath)}
Failed to parse the deploy configuration file at ${path.relative(".", deployConfigPath)}
${e instanceof ParseError ? formatMessage(e) : e}
`
);
}
if (!redirectedConfigPath) {
throw new UserError(dedent`
A redirect config was found at "${path.relative(".", deployConfigPath)}".
A deploy configuration file was found at "${path.relative(".", deployConfigPath)}".
But this is not valid - the required "configPath" property was not found.
Instead this file contains:
\`\`\`
Expand All @@ -111,8 +111,8 @@ function findRedirectedWranglerConfig(
if (redirectedConfigPath) {
if (!fs.existsSync(redirectedConfigPath)) {
throw new UserError(dedent`
There is a redirect configuration at "${path.relative(".", deployConfigPath)}".
But the config path it points to, "${path.relative(".", redirectedConfigPath)}", does not exist.
There is a deploy configuration at "${path.relative(".", deployConfigPath)}".
But the redirected configuration path it points to, "${path.relative(".", redirectedConfigPath)}", does not exist.
`);
}
if (userConfigPath) {
Expand All @@ -121,18 +121,18 @@ function findRedirectedWranglerConfig(
deployConfigPath
) {
throw new UserError(dedent`
Found both a user config file at "${path.relative(".", userConfigPath)}"
and a redirect config file at "${path.relative(".", deployConfigPath)}".
Found both a user configuration file at "${path.relative(".", userConfigPath)}"
and a deploy configuration file at "${path.relative(".", deployConfigPath)}".
But these do not share the same base path so it is not clear which should be used.
`);
}
}

logger.warn(dedent`
Using redirected Wrangler configuration.
Redirected config path: "${path.relative(".", redirectedConfigPath)}"
Deploy config path: "${path.relative(".", deployConfigPath)}"
Original config path: "${userConfigPath ? path.relative(".", userConfigPath) : "<no user config found>"}"
Configuration being used: "${path.relative(".", redirectedConfigPath)}"
Original user's configuration: "${userConfigPath ? path.relative(".", userConfigPath) : "<no user config found>"}"
Deploy configuration file: "${path.relative(".", deployConfigPath)}"
`);
return redirectedConfigPath;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/config/config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export type RawConfig = Partial<ConfigFields<RawDevConfig>> &
EnvironmentMap & { $schema?: string };

export interface ConfigFields<Dev extends RawDevConfig> {
/** The path to the Wrangler config file (if any, and possibly redirected from the user Wrangler configuration) used to create this configuration. */
/** The path to the Wrangler configuration file (if any, and possibly redirected from the user Wrangler configuration) used to create this configuration. */
configPath: string | undefined;
/** The path to the user's Wrangler config file (if any), which may have been redirected to the actual config used to create this configuration. */
/** The path to the user's Wrangler configuration file (if any), which may have been redirected to another file that used to create this configuration. */
userConfigPath: string | undefined;

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/config/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,14 @@ export function readPagesConfig(
} catch (e) {
logger.error(e);
throw new FatalError(
`Your ${configFileName(configPath)} file is not a valid Pages config file`,
`Your ${configFileName(configPath)} file is not a valid Pages configuration file`,
EXIT_CODE_INVALID_PAGES_CONFIG
);
}

if (!isPagesConfig(rawConfig)) {
throw new FatalError(
`Your ${configFileName(configPath)} file is not a valid Pages config file`,
`Your ${configFileName(configPath)} file is not a valid Pages configuration file`,
EXIT_CODE_INVALID_PAGES_CONFIG
);
}
Expand Down

0 comments on commit 0eea310

Please sign in to comment.