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: Config should be resolved relative to the entrypoint #447

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Feb 11, 2022

During dev and publish, we should resolve wrangler.toml starting from the entrypoint, and then working up from there. Currently, we start from the directory from which we call wrangler, this changes that behaviour to start from the entrypoint instead.

(To implement this, I made one big change: Inside commands, we now have to explicitly read configuration from a path, instead of expecting it to 'arrive' coerced into a configuration object.)

Fixes #440.

@changeset-bot
Copy link

changeset-bot bot commented Feb 11, 2022

🦋 Changeset detected

Latest commit: 7617b61

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

This PR includes changesets to release 1 package
Name Type
wrangler 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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

wrangler prerelease is available for testing:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/1838037310/wrangler

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Feb 12, 2022

OK, so this is what I am getting at. How about we write the beginning of the readConfig() to be like:

async function readConfig(
  configPath: string | undefined = "wrangler.toml",
  cwd = process.cwd()
): Promise<Config> {
  // TODO - terminate this early instead of going all the way to the root
  configPath = await findUp(configPath, { cwd });

  const config: Config = configPath
    ? TOML.parse(await readFile(configPath, "utf-8"))
    : {};

  ...
}

Then for most of the commands we can just run:

const configPath = args.config && path.resolve(args.config);
const config = await readConfig(configPath);

And for dev and publish we would use:

const configPath = args.config && path.resolve(args.config);
const config = await readConfig(configPath, args.script && path.dirname(args.script));

@threepointone
Copy link
Contributor Author

Could you explain why you think this is better? In your version, the signature is a bit confusing, because it implies the second arg is necessary even if configPath is truthy. I might be missing some context.

@petebacondarwin petebacondarwin added this to the 2.0 milestone Feb 13, 2022
During `dev` and `publish`, we should resolve `wrangler.toml` starting from the entrypoint, and then working up from there. Currently, we start from the directory from which we call `wrangler`, this changes that behaviour to start from the entrypoint instead.

(To implement this, I made one big change: Inside commands, we now have to explicitly read configuration from a path, instead of expecting it to 'arrive' coerced into a configuration object.)
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.

Could you explain why you think this is better? In your version, the signature is a bit confusing, because it implies the second arg is necessary even if configPath is truthy. I might be missing some context.

OK, so thinking about this some more. I am happy to drop my idea above. It does make the signature a bit weird.

Happy to go with what you have here. We could probably knock around a number of different approaches but they all have their limitations and this looks pretty robust.

@threepointone threepointone merged commit 2c5c934 into main Feb 14, 2022
@threepointone threepointone deleted the resolve-config branch February 14, 2022 10:21
@github-actions github-actions bot mentioned this pull request Feb 14, 2022
mrbbot added a commit that referenced this pull request Oct 31, 2023
* Respect `cf.cacheKey` in Cache API

* Fix caching of `ReadableStream`-bodied `Response`s

* Propagate `clock` through `GatewayFactory` for tests

* Catch loopback handler errors and return 500 response

* Add tests for `_getRangeResponse` and `_parseRanges` helpers

* Add `Miniflare` integration test helper

* Bump `undici` to `5.13.0` and add `@cloudflare/workers-types`

* Add Cache API tests
mrbbot added a commit that referenced this pull request Nov 1, 2023
* Respect `cf.cacheKey` in Cache API

* Fix caching of `ReadableStream`-bodied `Response`s

* Propagate `clock` through `GatewayFactory` for tests

* Catch loopback handler errors and return 500 response

* Add tests for `_getRangeResponse` and `_parseRanges` helpers

* Add `Miniflare` integration test helper

* Bump `undici` to `5.13.0` and add `@cloudflare/workers-types`

* Add Cache API tests
mrbbot added a commit that referenced this pull request Nov 1, 2023
* Respect `cf.cacheKey` in Cache API

* Fix caching of `ReadableStream`-bodied `Response`s

* Propagate `clock` through `GatewayFactory` for tests

* Catch loopback handler errors and return 500 response

* Add tests for `_getRangeResponse` and `_parseRanges` helpers

* Add `Miniflare` integration test helper

* Bump `undici` to `5.13.0` and add `@cloudflare/workers-types`

* Add Cache API tests
mrbbot added a commit that referenced this pull request Nov 1, 2023
* Respect `cf.cacheKey` in Cache API

* Fix caching of `ReadableStream`-bodied `Response`s

* Propagate `clock` through `GatewayFactory` for tests

* Catch loopback handler errors and return 500 response

* Add tests for `_getRangeResponse` and `_parseRanges` helpers

* Add `Miniflare` integration test helper

* Bump `undici` to `5.13.0` and add `@cloudflare/workers-types`

* Add Cache API tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config should be resolved relative to the entrypoint
2 participants