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

validate binding names don't conflict #665

Merged
merged 13 commits into from
Mar 24, 2022
Merged

Conversation

caass
Copy link
Contributor

@caass caass commented Mar 22, 2022

closes #658

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2022

🦋 Changeset detected

Latest commit: fdcbf3e

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 Mar 22, 2022

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2034567542/npm-package-wrangler-665

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

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/665/npm-package-wrangler-665

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2034567542/npm-package-wrangler-665 dev path/to/script.js

@caass caass force-pushed the unique-binding-names branch 2 times, most recently from 631289c to 0b55698 Compare March 22, 2022 19:37
@caass caass marked this pull request as ready for review March 22, 2022 19:49
@caass caass requested a review from threepointone as a code owner March 22, 2022 19:49
@caass caass marked this pull request as draft March 23, 2022 15:22
Cass Fridkin added 8 commits March 23, 2022 13:27
So...a bunch of tests started failing after I rebased. Turns out,
we maybe can't trust the inputs to `validateBindingsHaveUniqueNames`
to be valid since we don't actually _throw_ until we've performed
every config validation. So that means by the time we get to
`validateBindingsHaveUniqueNames`, we might have some invalid stuff
in the `Config` and we can't really trust the types. :(

So I've just gone back to using `getBindingNames` to get the binding
names, since it does all the null & type checking necessary to get
the binding names out from a binding.
@caass caass force-pushed the unique-binding-names branch from e8d6487 to 852ad20 Compare March 23, 2022 18:51
@caass caass marked this pull request as ready for review March 23, 2022 18:52
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.

Looks great!
We talked offline and there are some NITs that can be updated.
Approving in theory :-D

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.

Smashing!

packages/wrangler/src/config/validation-helpers.ts Outdated Show resolved Hide resolved
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.

Oh one tiny NIT.

.changeset/strange-dingos-listen.md Outdated Show resolved Hide resolved
Cass and others added 3 commits March 24, 2022 07:03
@petebacondarwin petebacondarwin merged commit 62a89c6 into main Mar 24, 2022
@petebacondarwin petebacondarwin deleted the unique-binding-names branch March 24, 2022 13:37
@github-actions github-actions bot mentioned this pull request Mar 24, 2022
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.

All the binding names should be distinct, no repeats (since they'll all be hanging off the same env object)
3 participants