Enums not tree shaking well

[jasonkuhrt]

I have redefined the Kind and OperationTypeNode enums in Graffle because they do not seem to tree shake well. You can see the code and bundle visualization in this PR. With my approach already just those two enums are reduced by over 4kb in Graffle.

Obviously it would be nice if graphql was already fully tree shake optimized from the start. Is there an appetite to adjust how the enums are defined in graphql? I think my approach would be a breaking change at the type level because TypeScript enum types, even if using string constants at runtime, are not equivalent to them at the type level.

Feel free to close this issue if there's zero chance of change on the graphql package side of course. Convesely, happy to help out on this issue.

[JoviDeCroock]

Yeah, there are several issues with the TS enum like #3356. However converting to an object won't solve your issue here, that being said, this is a reference implementation and we have made tradeoffs for DX/readability for that matter. The minzipped size of this object when not using TS enums should not exceed 1kb.

We could split this up between schema-kind and executable kind to improve this a bit more for web implementations.

[jasonkuhrt]

Right an object wouldn't solve tree shaking. Rather I defined every enum member as its own constant and then use the namespace to group them. That supports tree shaking and I see actually no DX issue here, since TS allows type names that match a namespace name so it ends up feeling identical to the enum before.

[JoviDeCroock]

Grouping them under a namespace won't allow them to be used in JS as follows right? I reckon that the use as type would be supported but use as value would not be. The issue is similar with using const enum you can use it as a type but not as a value.

import { Kind } from 'graphql';

if (selection.kind === Kind.FIELD)

My main concern would be to still allow this usage so v17 doesn't have too many breaking changes, in #4254 we move away from the enum notation which already saves 20% of the bytes in that file. I consider that to be a good start as it also allows for the type to be omitted and to just use the plain string, which in your case would help you get rid of the type-casting.

I would be a huge fan of evolving into a tree-shakable model though, for now I would advise people that are concerned with that to leverage your approach.

[jasonkuhrt]

Does the following address your example or am I missing something?

https://www.typescriptlang.org/dev/bug-workbench/?#code/FDD0oAgAQMwSwDYFMB2BDAtkgXBFB7AEyQH0MiBXZAZ1AHMAnNABwAsBHBUAazhUJ59CAOgAu1YEgAezfA1EQAxvhTUFAMQCSAUQAyAEQgBeCAHJ4SBIVOSZchctUKAQsbMAjUxHAQkoxcK2svJKKmoQAMJupopePn4BQfahThCGJqbW3pAJgUkhogCezEgQWnrpEEUl+DBlOgb5CtWlriYttRDOTVXFpVHtfZ0RPS1pbh11+iAgPrCIqJg4eESk5IRUSLSMLBxcvPygJCRiEnAYwc19EABUEGjUEADSQiQQMAz4GGbCgvzCACtqDZgGMXvxjhMhnVwYQSKNrrC3LDjgBtbhIQqdFFvABkEDUDD4dAAuiBpJdbvdHkiPl8fn8RECQbNIPNkOgsLgCMQyJQaPQmGxOKAhNJTj07nTvqZfgcBMdAcCZmA2fAOUtcCxmBLzpSAN7PIQQAC+70+Mp2woQIMc4Rg+HwbgAFPLcLCAJTGAB8EH1wAggYgcDqruNRgjZgsVlMXp8hCd1C+flYxKDEGAJpAsOE5QM2QgACJo4RC6zoOIALQUpCKUTVhifBjAB34Z2mdxoaxxyAwNCIECt9sl2MF-DcFuO9ueHsQceTtsxUc+edDzLLyDjoA

// @filename: node_modules/graphql/kind/kind.ts
export const FIELD = 'field'
export const B = 'b' // etc.
export const C = 'c' // etc.
export const D = 'd' // etc.

export type FIELD = typeof FIELD
export type B = typeof B
export type C = typeof C
export type D = typeof D



// @filename: node_modules/graphql/kind/__.ts
import type * as Kind_ from './kind.js'

type Kind__ = typeof Kind_
export type Kind = Kind__[keyof Kind__ & string]

export * as Kind from './kind.js'



// @filename: node_modules/graphql/index.ts
export * from './kind/__.js'



// @filename: app.ts
import { Kind } from 'graphql'

const foo = (kind: Kind) => {
    if (kind === 'field') // do something    
}

Kind.FIELD // "field"

// @ts-expect-error
foo('bad') // fail

foo('field') // ok
foo('b') // ok
foo('c') // ok
foo('d') // ok

[jasonkuhrt]

Here's another approach that might seem/be simpler:

https://www.typescriptlang.org/dev/bug-workbench/?#code/FDD0oAgAQMwSwDYFMB2BDAtkgXBFB7AEyQH0MiBXZAZ1AHMAnNABwAsBHBUAazhUJ59CAOgAu1YEgAezfA1EQAxvhTUFAMQCSAUQAyAEQgBeCAHJ4SBIVOSZchctUKAQsbMAjUxHAQkoxcK2svJKKmoQAMJupopePn4BQfahThCGJqbW3pAJgSBgkLCIqJg4eESk5IRUSLSMLBxcvPygJCRiEnAYwQoA3hBaevoANBDOoxGjhgC+EDAM+BhmwoL8wgBW1DZJIaIAnsxIEADSQsbAEBAAPhD7h-gwAzoGF9e3B0gPY683d5+PER+73uj30IGkPQgACoIGhqCczvNFstViJNtsQD4ish0FhcARiGRKDR6Ew2JxQEJpB0dgoYUilqYVs0BG0Nlt8gVoPAcaVcCxmDSupD+qd+BBZgyzPVyQhto5wjB8Pg3AAKFm4MWEACUxgAfBBeq9LnBHuqzkZLWYLFZTLqGH4KAwUNkIIQVdRFn5WHw6JdLsBpiAtcJBgZXQAiG2ECOYwriAC0EKQilESYYCwYwCV+FVpncaGsup8MDQiBAObz0btrvw3GzyrznmLkDrDdzMRrPjblcyXdb3CAA

// @filename: node_modules/graphql/kind/kind.ts
export const FIELD = 'field'
export const B = 'b' // etc.
export const C = 'c' // etc.
export const D = 'd' // etc.



// @filename: node_modules/graphql/kind/__.ts
import { FIELD, B, C, D } from './kind.js'

export type Kind =
  | typeof FIELD
  | typeof B
  | typeof C
  | typeof D

export * as Kind from './kind.js'



// @filename: node_modules/graphql/index.ts
export * from './kind/__.js'



// @filename: app.ts
import { Kind } from 'graphql'

const foo = (kind: Kind) => {
    if (kind === 'field') return // do something    
}

Kind.FIELD // "field"

// @ts-expect-error
foo('bad') // fail

foo('field') // ok
foo('b') // ok
foo('c') // ok
foo('d') // ok

[jasonkuhrt]

It is worth another check but my understanding is that this allows app.ts to tree shake graphql Kind down to JUST what it uses from the Kind namespace.

[JoviDeCroock]

Well you can't then use i.e. OperationTypeNode as a type as it would become a namespace. The tree-shaking does look to be working correctly so we could do this for Kind in and of itself and let #4254 handle the bundle reduction for the ones we use as a type.

I reckon that the CJS equivalent might tree-shake less elegantly but the more reason for leveraging ESM.

EDIT: I tried this out and the using the namespace as a type keeps failing as when we use typeof namespace it fails again - you can check it out here https://github.com/graphql/graphql-js/pull/new/tree-shake-kind

[jasonkuhrt]

Well you can't then use i.e. OperationTypeNode as a type as it would become a namespace.

Hey @JoviDeCroock, I'm not sure what you mean by this.

In my example above for example we see that Kind is both namespace and type. TypeScript allows this.

// @filename: app.ts
import { Kind } from 'graphql' // <-- `Kind` is at both type & term levels

EDIT: I tried this out and the using the namespace as a type keeps failing as when we use typeof namespace it fails again - you can check it out here graphql/graphql-js/pull/new/tree-shake-kind

Thanks will check it out!

[jasonkuhrt]

I will have a PR up today that we can discus further.

[JoviDeCroock]

Fixed it in #4268

[jasonkuhrt]

@JoviDeCroock Ah beat me to it. PR is at fwiw #4269 but I'll close it now. Thanks!

[jasonkuhrt]

Reopening. Sorry didn't realize the PR is still open.

I dropped some feedback on the PR.

[JoviDeCroock]

I'll close this as we've merged this into the v17 branch