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

prefer worker package export conditionals if available #93

Merged
merged 8 commits into from Dec 15, 2021

Conversation

nicksrandall
Copy link
Contributor

Related discussion in #84

@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2021

🦋 Changeset detected

Latest commit: 6470f98

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

@threepointone
Copy link
Contributor

Thanks for the PR! I want to discuss internally what we want it to be called ("cloudflare-worker" feels a bit long to me, but I dunno what a good replacement is just yet.). I'll get back to this when we have some clarity.

@nicksrandall
Copy link
Contributor Author

@threepointone do you think it would make sense to land this now with only support for "worker" condition and then we can add in a more specific condition for "cloudflare-worker" (whatever we decide to name it) in the future? If so, I can update this PR to reflect that change.

@threepointone
Copy link
Contributor

Yeah, sounds good to me, let's do that.

@nicksrandall
Copy link
Contributor Author

Done. Let me know if there is anything else you'd like me to change.

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

Thanks for this! Left some comments. I'll test this out tomorrow locally and merge it.

packages/wrangler/src/dev.tsx Show resolved Hide resolved
packages/wrangler/src/dev.tsx Outdated Show resolved Hide resolved
packages/wrangler/src/publish.ts Outdated Show resolved Hide resolved
@threepointone threepointone changed the title prefer cloudflare-worker then worker package export conditionals if available prefer worker package export conditionals if available Dec 14, 2021
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

This looks fine to me. @Andarist, what do you think? should we add webworker as well?

@Andarist
Copy link
Contributor

Andarist commented Dec 15, 2021

This looks fine to me. @Andarist, what do you think?

LGTM too - I've only left some minor suggestions.

should we add webworker as well?

I don't think you should include webworker condition yet - cause this has not been defined/promoted anywhere (at least, not just yet, and as far as I've seen, I could have missed something).

I believe there were some talks about creating a document that would gather all known "standardized" conditions, one that would live outside of the node/webpack/whatever. I'm not sure what is the status of this though - cc @guybedford

threepointone and others added 2 commits December 15, 2021 14:35
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

Thanks to the both of you! Made the changes, landing this.

@threepointone threepointone merged commit e928f94 into cloudflare:main Dec 15, 2021
GregBrimble pushed a commit that referenced this pull request Feb 8, 2024
This switch/typeof example isn't valid JavaScript AFAICT -- JS doesn't have "type switches" like Go does.

NOTE: Unfortunately this example still won't work, because NotFoundError and MethodNotAllowedError are not imported. It doesn't look like kv-asset-handler exports these types currently, so I guess a code change is needed to export them?

See #93 (not fully fixed by this PR)
GregBrimble pushed a commit that referenced this pull request Feb 9, 2024
This switch/typeof example isn't valid JavaScript AFAICT -- JS doesn't have "type switches" like Go does.

NOTE: Unfortunately this example still won't work, because NotFoundError and MethodNotAllowedError are not imported. It doesn't look like kv-asset-handler exports these types currently, so I guess a code change is needed to export them?

See #93 (not fully fixed by this PR)
GregBrimble pushed a commit that referenced this pull request Feb 9, 2024
This switch/typeof example isn't valid JavaScript AFAICT -- JS doesn't have "type switches" like Go does.

NOTE: Unfortunately this example still won't work, because NotFoundError and MethodNotAllowedError are not imported. It doesn't look like kv-asset-handler exports these types currently, so I guess a code change is needed to export them?

See #93 (not fully fixed by this PR)
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.

None yet

3 participants