implement bundling changes suggested in #4062 #4096

phryneas · 2024-05-24T14:10:49Z

This is now ready for review.

Current AreTheTypesWrong output:

 graphql v16.7.0

 No problems found 🌟


"graphql/language/ast.js"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/language/parser.js"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/jsutils/instanceOf.js"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/execution/execute.js"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/error"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/execution"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/language"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/subscription"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/type"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/utilities"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/validation"

node10: 🟢 
node16 (from CJS): 🟢 (CJS)
node16 (from ESM): 🟢 (ESM)
bundler: 🟢 

***********************************

"graphql/package.json"

node10: 🟢 (JSON)
node16 (from CJS): 🟢 (JSON)
node16 (from ESM): 🟢 (JSON)
bundler: 🟢 (JSON)

***********************************

Running publint also is successful.

resources/build-npm.js

package.json

resources/build-npm.js

phryneas · 2024-05-24T14:23:59Z

Generated files look like this:

index.js.d.mts

export {
  version,
  versionInfo,
  graphql,
  graphqlSync,
  resolveObjMapThunk,
  resolveReadonlyArrayThunk,
  GraphQLSchema,
  GraphQLDirective,
  GraphQLScalarType,
  GraphQLObjectType,
  GraphQLInterfaceType,
  GraphQLUnionType,
  GraphQLEnumType,
  GraphQLInputObjectType,
  GraphQLList,
  GraphQLNonNull,
  specifiedScalarTypes,
  GraphQLInt,
  GraphQLFloat,
  GraphQLString,
  GraphQLBoolean,
  GraphQLID,
  GRAPHQL_MAX_INT,
  GRAPHQL_MIN_INT,
  specifiedDirectives,
  GraphQLIncludeDirective,
  GraphQLSkipDirective,
  GraphQLDeprecatedDirective,
  GraphQLSpecifiedByDirective,
  TypeKind,
  DEFAULT_DEPRECATION_REASON,
  introspectionTypes,
  __Schema,
  __Directive,
  __DirectiveLocation,
  __Type,
  __Field,
  __InputValue,
  __EnumValue,
  __TypeKind,
  SchemaMetaFieldDef,
  TypeMetaFieldDef,
  TypeNameMetaFieldDef,
  isSchema,
  isDirective,
  isType,
  isScalarType,
  isObjectType,
  isInterfaceType,
  isUnionType,
  isEnumType,
  isInputObjectType,
  isListType,
  isNonNullType,
  isInputType,
  isOutputType,
  isLeafType,
  isCompositeType,
  isAbstractType,
  isWrappingType,
  isNullableType,
  isNamedType,
  isRequiredArgument,
  isRequiredInputField,
  isSpecifiedScalarType,
  isIntrospectionType,
  isSpecifiedDirective,
  assertSchema,
  assertDirective,
  assertType,
  assertScalarType,
  assertObjectType,
  assertInterfaceType,
  assertUnionType,
  assertEnumType,
  assertInputObjectType,
  assertListType,
  assertNonNullType,
  assertInputType,
  assertOutputType,
  assertLeafType,
  assertCompositeType,
  assertAbstractType,
  assertWrappingType,
  assertNullableType,
  assertNamedType,
  getNullableType,
  getNamedType,
  validateSchema,
  assertValidSchema,
  assertName,
  assertEnumValueName,
  Token,
  Source,
  Location,
  OperationTypeNode,
  getLocation,
  printLocation,
  printSourceLocation,
  Lexer,
  TokenKind,
  parse,
  parseValue,
  parseConstValue,
  parseType,
  print,
  visit,
  visitInParallel,
  getVisitFn,
  getEnterLeaveForKind,
  BREAK,
  Kind,
  DirectiveLocation,
  isDefinitionNode,
  isExecutableDefinitionNode,
  isSelectionNode,
  isValueNode,
  isConstValueNode,
  isTypeNode,
  isTypeSystemDefinitionNode,
  isTypeDefinitionNode,
  isTypeSystemExtensionNode,
  isTypeExtensionNode,
  execute,
  executeSync,
  defaultFieldResolver,
  defaultTypeResolver,
  responsePathAsArray,
  getArgumentValues,
  getVariableValues,
  getDirectiveValues,
  subscribe,
  createSourceEventStream,
  validate,
  ValidationContext,
  specifiedRules,
  ExecutableDefinitionsRule,
  FieldsOnCorrectTypeRule,
  FragmentsOnCompositeTypesRule,
  KnownArgumentNamesRule,
  KnownDirectivesRule,
  KnownFragmentNamesRule,
  KnownTypeNamesRule,
  LoneAnonymousOperationRule,
  NoFragmentCyclesRule,
  NoUndefinedVariablesRule,
  NoUnusedFragmentsRule,
  NoUnusedVariablesRule,
  OverlappingFieldsCanBeMergedRule,
  PossibleFragmentSpreadsRule,
  ProvidedRequiredArgumentsRule,
  ScalarLeafsRule,
  SingleFieldSubscriptionsRule,
  UniqueArgumentNamesRule,
  UniqueDirectivesPerLocationRule,
  UniqueFragmentNamesRule,
  UniqueInputFieldNamesRule,
  UniqueOperationNamesRule,
  UniqueVariableNamesRule,
  ValuesOfCorrectTypeRule,
  VariablesAreInputTypesRule,
  VariablesInAllowedPositionRule,
  LoneSchemaDefinitionRule,
  UniqueOperationTypesRule,
  UniqueTypeNamesRule,
  UniqueEnumValueNamesRule,
  UniqueFieldDefinitionNamesRule,
  UniqueArgumentDefinitionNamesRule,
  UniqueDirectiveNamesRule,
  PossibleTypeExtensionsRule,
  NoDeprecatedCustomRule,
  NoSchemaIntrospectionCustomRule,
  GraphQLError,
  syntaxError,
  locatedError,
  printError,
  formatError,
  getIntrospectionQuery,
  getOperationAST,
  getOperationRootType,
  introspectionFromSchema,
  buildClientSchema,
  buildASTSchema,
  buildSchema,
  extendSchema,
  lexicographicSortSchema,
  printSchema,
  printType,
  printIntrospectionSchema,
  typeFromAST,
  valueFromAST,
  valueFromASTUntyped,
  astFromValue,
  TypeInfo,
  visitWithTypeInfo,
  coerceInputValue,
  concatAST,
  separateOperations,
  stripIgnoredCharacters,
  isEqualType,
  isTypeSubTypeOf,
  doTypesOverlap,
  assertValidName,
  isValidNameError,
  BreakingChangeType,
  DangerousChangeType,
  findBreakingChanges,
  findDangerousChanges,
} from './index.js';
export type {
  GraphQLArgs,
  GraphQLType,
  GraphQLInputType,
  GraphQLOutputType,
  GraphQLLeafType,
  GraphQLCompositeType,
  GraphQLAbstractType,
  GraphQLWrappingType,
  GraphQLNullableType,
  GraphQLNamedType,
  GraphQLNamedInputType,
  GraphQLNamedOutputType,
  ThunkReadonlyArray,
  ThunkObjMap,
  GraphQLSchemaConfig,
  GraphQLSchemaExtensions,
  GraphQLDirectiveConfig,
  GraphQLDirectiveExtensions,
  GraphQLArgument,
  GraphQLArgumentConfig,
  GraphQLArgumentExtensions,
  GraphQLEnumTypeConfig,
  GraphQLEnumTypeExtensions,
  GraphQLEnumValue,
  GraphQLEnumValueConfig,
  GraphQLEnumValueConfigMap,
  GraphQLEnumValueExtensions,
  GraphQLField,
  GraphQLFieldConfig,
  GraphQLFieldConfigArgumentMap,
  GraphQLFieldConfigMap,
  GraphQLFieldExtensions,
  GraphQLFieldMap,
  GraphQLFieldResolver,
  GraphQLInputField,
  GraphQLInputFieldConfig,
  GraphQLInputFieldConfigMap,
  GraphQLInputFieldExtensions,
  GraphQLInputFieldMap,
  GraphQLInputObjectTypeConfig,
  GraphQLInputObjectTypeExtensions,
  GraphQLInterfaceTypeConfig,
  GraphQLInterfaceTypeExtensions,
  GraphQLIsTypeOfFn,
  GraphQLObjectTypeConfig,
  GraphQLObjectTypeExtensions,
  GraphQLResolveInfo,
  ResponsePath,
  GraphQLScalarTypeConfig,
  GraphQLScalarTypeExtensions,
  GraphQLTypeResolver,
  GraphQLUnionTypeConfig,
  GraphQLUnionTypeExtensions,
  GraphQLScalarSerializer,
  GraphQLScalarValueParser,
  GraphQLScalarLiteralParser,
  ParseOptions,
  SourceLocation,
  TokenKindEnum,
  KindEnum,
  DirectiveLocationEnum,
  ASTVisitor,
  ASTVisitFn,
  ASTVisitorKeyMap,
  ASTNode,
  ASTKindToNode,
  NameNode,
  DocumentNode,
  DefinitionNode,
  ExecutableDefinitionNode,
  OperationDefinitionNode,
  VariableDefinitionNode,
  VariableNode,
  SelectionSetNode,
  SelectionNode,
  FieldNode,
  ArgumentNode,
  ConstArgumentNode,
  FragmentSpreadNode,
  InlineFragmentNode,
  FragmentDefinitionNode,
  ValueNode,
  ConstValueNode,
  IntValueNode,
  FloatValueNode,
  StringValueNode,
  BooleanValueNode,
  NullValueNode,
  EnumValueNode,
  ListValueNode,
  ConstListValueNode,
  ObjectValueNode,
  ConstObjectValueNode,
  ObjectFieldNode,
  ConstObjectFieldNode,
  DirectiveNode,
  ConstDirectiveNode,
  TypeNode,
  NamedTypeNode,
  ListTypeNode,
  NonNullTypeNode,
  TypeSystemDefinitionNode,
  SchemaDefinitionNode,
  OperationTypeDefinitionNode,
  TypeDefinitionNode,
  ScalarTypeDefinitionNode,
  ObjectTypeDefinitionNode,
  FieldDefinitionNode,
  InputValueDefinitionNode,
  InterfaceTypeDefinitionNode,
  UnionTypeDefinitionNode,
  EnumTypeDefinitionNode,
  EnumValueDefinitionNode,
  InputObjectTypeDefinitionNode,
  DirectiveDefinitionNode,
  TypeSystemExtensionNode,
  SchemaExtensionNode,
  TypeExtensionNode,
  ScalarTypeExtensionNode,
  ObjectTypeExtensionNode,
  InterfaceTypeExtensionNode,
  UnionTypeExtensionNode,
  EnumTypeExtensionNode,
  InputObjectTypeExtensionNode,
  ExecutionArgs,
  ExecutionResult,
  FormattedExecutionResult,
  SubscriptionArgs,
  ValidationRule,
  GraphQLErrorOptions,
  GraphQLFormattedError,
  GraphQLErrorExtensions,
  IntrospectionOptions,
  IntrospectionQuery,
  IntrospectionSchema,
  IntrospectionType,
  IntrospectionInputType,
  IntrospectionOutputType,
  IntrospectionScalarType,
  IntrospectionObjectType,
  IntrospectionInterfaceType,
  IntrospectionUnionType,
  IntrospectionEnumType,
  IntrospectionInputObjectType,
  IntrospectionTypeRef,
  IntrospectionInputTypeRef,
  IntrospectionOutputTypeRef,
  IntrospectionNamedTypeRef,
  IntrospectionListTypeRef,
  IntrospectionNonNullTypeRef,
  IntrospectionField,
  IntrospectionInputValue,
  IntrospectionEnumValue,
  IntrospectionDirective,
  BuildSchemaOptions,
  BreakingChange,
  DangerousChange,
  TypedQueryDocumentNode,
} from './index.js';

index.js.mjs

export {
  version,
  versionInfo,
  graphql,
  graphqlSync,
  resolveObjMapThunk,
  resolveReadonlyArrayThunk,
  GraphQLSchema,
  GraphQLDirective,
  GraphQLScalarType,
  GraphQLObjectType,
  GraphQLInterfaceType,
  GraphQLUnionType,
  GraphQLEnumType,
  GraphQLInputObjectType,
  GraphQLList,
  GraphQLNonNull,
  specifiedScalarTypes,
  GraphQLInt,
  GraphQLFloat,
  GraphQLString,
  GraphQLBoolean,
  GraphQLID,
  GRAPHQL_MAX_INT,
  GRAPHQL_MIN_INT,
  specifiedDirectives,
  GraphQLIncludeDirective,
  GraphQLSkipDirective,
  GraphQLDeprecatedDirective,
  GraphQLSpecifiedByDirective,
  TypeKind,
  DEFAULT_DEPRECATION_REASON,
  introspectionTypes,
  __Schema,
  __Directive,
  __DirectiveLocation,
  __Type,
  __Field,
  __InputValue,
  __EnumValue,
  __TypeKind,
  SchemaMetaFieldDef,
  TypeMetaFieldDef,
  TypeNameMetaFieldDef,
  isSchema,
  isDirective,
  isType,
  isScalarType,
  isObjectType,
  isInterfaceType,
  isUnionType,
  isEnumType,
  isInputObjectType,
  isListType,
  isNonNullType,
  isInputType,
  isOutputType,
  isLeafType,
  isCompositeType,
  isAbstractType,
  isWrappingType,
  isNullableType,
  isNamedType,
  isRequiredArgument,
  isRequiredInputField,
  isSpecifiedScalarType,
  isIntrospectionType,
  isSpecifiedDirective,
  assertSchema,
  assertDirective,
  assertType,
  assertScalarType,
  assertObjectType,
  assertInterfaceType,
  assertUnionType,
  assertEnumType,
  assertInputObjectType,
  assertListType,
  assertNonNullType,
  assertInputType,
  assertOutputType,
  assertLeafType,
  assertCompositeType,
  assertAbstractType,
  assertWrappingType,
  assertNullableType,
  assertNamedType,
  getNullableType,
  getNamedType,
  validateSchema,
  assertValidSchema,
  assertName,
  assertEnumValueName,
  Token,
  Source,
  Location,
  OperationTypeNode,
  getLocation,
  printLocation,
  printSourceLocation,
  Lexer,
  TokenKind,
  parse,
  parseValue,
  parseConstValue,
  parseType,
  print,
  visit,
  visitInParallel,
  getVisitFn,
  getEnterLeaveForKind,
  BREAK,
  Kind,
  DirectiveLocation,
  isDefinitionNode,
  isExecutableDefinitionNode,
  isSelectionNode,
  isValueNode,
  isConstValueNode,
  isTypeNode,
  isTypeSystemDefinitionNode,
  isTypeDefinitionNode,
  isTypeSystemExtensionNode,
  isTypeExtensionNode,
  execute,
  executeSync,
  defaultFieldResolver,
  defaultTypeResolver,
  responsePathAsArray,
  getArgumentValues,
  getVariableValues,
  getDirectiveValues,
  subscribe,
  createSourceEventStream,
  validate,
  ValidationContext,
  specifiedRules,
  ExecutableDefinitionsRule,
  FieldsOnCorrectTypeRule,
  FragmentsOnCompositeTypesRule,
  KnownArgumentNamesRule,
  KnownDirectivesRule,
  KnownFragmentNamesRule,
  KnownTypeNamesRule,
  LoneAnonymousOperationRule,
  NoFragmentCyclesRule,
  NoUndefinedVariablesRule,
  NoUnusedFragmentsRule,
  NoUnusedVariablesRule,
  OverlappingFieldsCanBeMergedRule,
  PossibleFragmentSpreadsRule,
  ProvidedRequiredArgumentsRule,
  ScalarLeafsRule,
  SingleFieldSubscriptionsRule,
  UniqueArgumentNamesRule,
  UniqueDirectivesPerLocationRule,
  UniqueFragmentNamesRule,
  UniqueInputFieldNamesRule,
  UniqueOperationNamesRule,
  UniqueVariableNamesRule,
  ValuesOfCorrectTypeRule,
  VariablesAreInputTypesRule,
  VariablesInAllowedPositionRule,
  LoneSchemaDefinitionRule,
  UniqueOperationTypesRule,
  UniqueTypeNamesRule,
  UniqueEnumValueNamesRule,
  UniqueFieldDefinitionNamesRule,
  UniqueArgumentDefinitionNamesRule,
  UniqueDirectiveNamesRule,
  PossibleTypeExtensionsRule,
  NoDeprecatedCustomRule,
  NoSchemaIntrospectionCustomRule,
  GraphQLError,
  syntaxError,
  locatedError,
  printError,
  formatError,
  getIntrospectionQuery,
  getOperationAST,
  getOperationRootType,
  introspectionFromSchema,
  buildClientSchema,
  buildASTSchema,
  buildSchema,
  extendSchema,
  lexicographicSortSchema,
  printSchema,
  printType,
  printIntrospectionSchema,
  typeFromAST,
  valueFromAST,
  valueFromASTUntyped,
  astFromValue,
  TypeInfo,
  visitWithTypeInfo,
  coerceInputValue,
  concatAST,
  separateOperations,
  stripIgnoredCharacters,
  isEqualType,
  isTypeSubTypeOf,
  doTypesOverlap,
  assertValidName,
  isValidNameError,
  BreakingChangeType,
  DangerousChangeType,
  findBreakingChanges,
  findDangerousChanges,
} from './index.js';

phryneas · 2024-05-27T10:48:25Z

I just noticed that the folders are declared as official entry points:

 * This also includes utility functions for operating on GraphQL types and
 * GraphQL documents to facilitate building tools.
 *
 * You may also import from each sub-directory directly. For example, the
 * following two import statements are equivalent:
 *
 * ```ts
 * import { parse } from 'graphql';
 * import { parse } from 'graphql/language';
 * ```

phryneas · 2024-05-27T13:31:44Z

This still ended up as a dual module hazard in vitest, so I had to swap import and module, which now works:

         "import": "./jsutils/instanceOf.js.d.mts",
         "default": "./jsutils/instanceOf.d.ts"
       },
-      "module": "./jsutils/instanceOf.mjs",
       "import": "./jsutils/instanceOf.js.mjs",
+      "module": "./jsutils/instanceOf.mjs",
       "default": "./jsutils/instanceOf.js"
     },

The question is just if that has any unintended side effects... Asking Andarist on the side :)

package.json

phryneas · 2024-05-27T13:37:29Z

Just documenting this little nugget of knowledge before I never find it again:
vitest-dev/vitest#4233 (reply in thread)

Vitest uses the same mechanism to resolve module with two exceptions. Vitest doesn't look at top-level "module" field in package.json, and it prioritizes "node" exports condition. This is needed so you won't have two separate modules imported in cases one of the modules is imported using Native Node resolver (which we can't modify to support "module" field without a big performance hit)

phryneas · 2024-05-27T14:06:56Z

Moving import before module seemed too dangerous - it would probably point too many bundlers at the CJS, not the ESM.

So instead, I've added a ~~vitest~~ "dual-module-hazard-workaround" condition. It shouldn't impact any other environment, but we can tell vitest users to adjust their configuration.

import { defineConfig } from 'vite';
export default defineConfig(async ({ mode }) => {
  return {
    resolve: mode === 'test' ? { conditions: ["dual-module-hazard-workaround"] } : undefined,
  };
});

This would allow vitest users to avoid the dual package hazard without breaking anything else.

resources/build-npm.js

phryneas · 2024-05-28T12:21:38Z

integrationTests/ts/package.json

@@ -6,11 +6,18 @@
  },
  "dependencies": {
    "graphql": "file:../graphql.tgz",
-    "typescript-4.1": "npm:typescript@4.1.x",


These changes break TS 4.1, but that would probably be okay for a new major of graphql.

what exactly causes them to break TS 4.1 out of curiosity?

do we have to remove 4.3 through 4.8?

I was going with the DefinitelyTyped support window for TypeScript versions here.

No other reason to drop them than that not even Microsoft themself want to maintain types for those old versions any more - it seemed reasonable as I had to touch this already, and we won't be able to drop any TS versions between majors, so if we even want to remotely consider using modern features in the future, this would be the time.

As for 4.1, it doesn't resolve the exports, while all other versions do.

Tons of

basic-test.ts:1:38 - error TS7016: Could not find a declaration file for module 'graphql/execution'. '/private/var/folders/ms/w25rss0n4vq830bvptjjkqvr0000gp/T/graphql-js-integrationTmp/ts/node_modules/graphql/execution/index.js' implicitly has an 'any' type. Try `npm i --save-dev @types/graphql` if it exists or add a new declaration (.d.ts) file containing `declare module 'graphql/execution';` 1 import type { ExecutionResult } from 'graphql/execution'; ~~~~~~~~~~~~~~~~~~~ basic-test.ts:3:29 - error TS7016: Could not find a declaration file for module 'graphql'. '/private/var/folders/ms/w25rss0n4vq830bvptjjkqvr0000gp/T/graphql-js-integrationTmp/ts/node_modules/graphql/index.js' implicitly has an 'any' type. Try `npm i --save-dev @types/graphql` if it exists or add a new declaration (.d.ts) file containing `declare module 'graphql';` 3 import { graphqlSync } from 'graphql'; ~~~~~~~~~ basic-test.ts:4:65 - error TS7016: Could not find a declaration file for module 'graphql/type'. '/private/var/folders/ms/w25rss0n4vq830bvptjjkqvr0000gp/T/graphql-js-integrationTmp/ts/node_modules/graphql/type/index.js' implicitly has an 'any' type. Try `npm i --save-dev @types/graphql` if it exists or add a new declaration (.d.ts) file containing `declare module 'graphql/type';` 4 import { GraphQLString, GraphQLSchema, GraphQLObjectType } from 'graphql/type'; ~~~~~~~~~~~~~~

etc.

Ah. Then why don’t we remove support in a separate PR so it goes in as a separate breaking change

phryneas · 2024-05-28T12:22:03Z

integrationTests/ts/package.json

+    "typescript-5.0": "npm:typescript@5.0.x",
+    "typescript-5.1": "npm:typescript@5.1.x",
+    "typescript-5.2": "npm:typescript@5.2.x",
+    "typescript-5.3": "npm:typescript@5.3.x",
+    "typescript-5.4": "npm:typescript@5.4.x"


How many versions should be supported here?

I've chosen the same range as DefinitelyTyped for now: https://github.com/DefinitelyTyped/DefinitelyTyped?tab=readme-ov-file#support-window (>= 4.8)

We might choose to enforce more modern versions, though.

this has been updated in main, maybe we can rebase this PR against main?

phryneas · 2024-07-29T14:10:20Z

resources/build-npm.js

+    'execution/execute.ts',
+    'jsutils/instanceOf.ts',
+    'language/parser.ts',
+    'language/ast.ts',


These have been chosen by gut feeling out of the exports mentioned in #4074

We need to generally decide on a way forward here:

expose those .js file?

instead, export these exports from the parent "nested" entrypoint?

export them from the main entrypoint?

Labelling a function with export but burying it so users would have to deep import it was a way to allow entryway into our private API without issuing any semver guarantee => a convenience to power users, so to speak.

With explicit exports, with option 1 and 2, we kind of lose that distinction, and it's much more difficult to garden off these bits as a private API that happens to be exposed.

So I vote that we examine each in term, and make the somewhat difficult decision of whether to support semver on them, and in that case go with option C, export from main entrypoint, or no longer expose them at all.

Yeah, I feel similarly.

I would suggest that I remove all of these exports and the annotations I made in this PR, and we open a new PR to expose these after deciding which we want to expose?

yaacovCR

Should the base for these changes be 16.x.x => are we trying to get this into v16 (current base branch: 16.x.x) or v17 (proposed base branch: main)?

yaacovCR · 2024-09-11T11:47:37Z

integrationTests/ts/package.json

@@ -6,11 +6,18 @@
  },
  "dependencies": {
    "graphql": "file:../graphql.tgz",
-    "typescript-4.1": "npm:typescript@4.1.x",


what exactly causes them to break TS 4.1 out of curiosity?

do we have to remove 4.3 through 4.8?

yaacovCR · 2024-09-11T12:04:55Z

resources/build-npm.js

+    'execution/execute.ts',
+    'jsutils/instanceOf.ts',
+    'language/parser.ts',
+    'language/ast.ts',


Labelling a function with export but burying it so users would have to deep import it was a way to allow entryway into our private API without issuing any semver guarantee => a convenience to power users, so to speak.

With explicit exports, with option 1 and 2, we kind of lose that distinction, and it's much more difficult to garden off these bits as a private API that happens to be exposed.

So I vote that we examine each in term, and make the somewhat difficult decision of whether to support semver on them, and in that case go with option C, export from main entrypoint, or no longer expose them at all.

phryneas · 2024-09-11T13:59:05Z

@yaacovCR

Should the base for these changes be 16.x.x => are we trying to get this into v16 (current base branch: 16.x.x) or v17 (proposed base branch: main)?

This was targeting the 16.x branch because at the time of discussing this in the WG, it wasn't clear when a release of the current v17 alphas as stable would be possible.
So @benjie suggested that we release this as v17, and move the current v17 alphas to v18.

If this has changed by now I can also target the v17 branch.

Notes here: https://github.com/graphql/graphql-js-wg/blob/main/notes/2024/2024-04.md

yaacovCR · 2024-09-11T14:14:50Z

If this has changed by now I can also target the v17 branch.

My feeling is that this has changed but perhaps @JoviDeCroock and @benjie might want to weigh in directly.

benjie · 2024-09-18T16:45:25Z

This is a breaking change so, when accepted, it should go in the next major release. Ideally whether incremental delivery goes out in the next major release or not should be a separate question - I don't think we need to tie these two features together - but since it sounds from Yaacov like main is nearing release readiness, merging to main would make sense I guess.

I'm a little hesitant because incremental delivery has had a big impact on the codebase, even when turned off, and that warrants careful scrutiny.

yaacovCR · 2024-09-24T10:21:10Z

Just documenting this little nugget of knowledge before I never find it again: vitest-dev/vitest#4233 (reply in thread)

Vitest uses the same mechanism to resolve module with two exceptions. Vitest doesn't look at top-level "module" field in package.json, and it prioritizes "node" exports condition. This is needed so you won't have two separate modules imported in cases one of the modules is imported using Native Node resolver (which we can't modify to support "module" field without a big performance hit)

@phryneas just thinking about this more, just want to check if I understand things correctly, the dual packages when using vitest stem from what exactly? I am guessing it is because vitest (1) uses the vite dev server (which uses esbuild vs. rollup vs both?) and picks up by default the build from the "module" condition and gets the pure esm package, while (2) vitest itself as a node test runner uses the import/default conditions?

And so by adding this specific condition, we are telling esbuild via vite that for every other package, you can go ahead and continue to use the module condition, but with graphql-js you get a dual package hazard, so don't?

If I understand things correctly, vitest will still give you dual package hazards for every other affected library besides graphql-js?

Within esbuild itself, if you set any custom condition, or even an empty list, esbuild will not add the "module" condition. I am not sure if that carries through when vite calls whatever it calls in its pipeline? But if it does, then when we tell them to set our custom condition, then they never get module respected for any package? https://esbuild.github.io/api/#platform That would be odd.

If no custom conditions are configured, the Webpack-specific module condition is also included. The module condition is used by package authors to provide a tree-shakable ESM alternative to a CommonJS file without creating a dual package hazard. You can prevent the module condition from being included by explicitly configuring some custom conditions (even an empty list).

Another workaround for vitest users is the suggestion here for modifying how vite resolves graphql-js, which will have a parallel for other affected libraries, but definitely not affect them? vitest-dev/vitest#4233 (comment)

I may have most of the above wrong -- would love more information. Overall, I would love to avoid putting this dual-package-hazard condition in here, as it seems pretty confusing, the rest of the setup is designed to avoid the dual-package-hazard => this seems like more of an honest-to-goodness tricky bug within vitest and its dependencies.

yaacovCR · 2024-09-25T12:37:23Z

To supplement: there is a meta-issue documenting some of the problems with vitests's approach and some of the root causes and workarounds: vitest-dev/vitest#5486

I am leaning toward not trying to fix this with a workaround that switches vitest from using esm to our esm=>cjs wrapper, but just using the workarounds discussed there.

yaacovCR · 2024-11-27T12:23:49Z

@phryneas => do you have any interest in rebasing this on the v17 branch in main => if not, I might be able to take a shot at it. Thanks for working on this!

phryneas · 2024-11-27T19:04:16Z

I won't be able to get to this within the next 2-3 weeks or so - after that, I could give it a try.
But I also wouldn't mind you to take over - your call :)

phryneas added 3 commits May 24, 2024 12:20

annotate files with known used exports

85ecc7a

generate .js.mjs and .js.d.mts files

9646d84

adjust package.json

16fa5d5

phryneas commented May 24, 2024

View reviewed changes

resources/build-npm.js Outdated Show resolved Hide resolved

phryneas commented May 24, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

phryneas commented May 24, 2024

View reviewed changes

resources/build-npm.js Show resolved Hide resolved

phryneas added 2 commits May 27, 2024 14:59

add entry point for each folder, generate exports

2fa37dc

flip order of module/import

881b1bf

phryneas commented May 27, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

roll back, add vitest condition

864015c

phryneas commented May 27, 2024

View reviewed changes

resources/build-npm.js Outdated Show resolved Hide resolved

phryneas added 2 commits May 28, 2024 14:07

add package.json to exports field

f93c7bf

bump integration test ts versions

011ce2c

phryneas commented May 28, 2024

View reviewed changes

adjust linter warnings

f86e6d0

phryneas mentioned this pull request May 29, 2024

Clean up bundling changes & mark them ready for review graphql/graphql-js-wg#124

Closed

phryneas mentioned this pull request Jun 25, 2024

Suggestion: Bundling in v17, ESM, CJS, and the dual package hazard #4062

Open

1 task

phryneas added 2 commits July 29, 2024 15:32

rename vitest condition to 'dual-module-hazard-workaround'

73adce6

apply definitelyTyped TS support

44516d2

phryneas force-pushed the 4062-bundling-changes branch from 14dc95a to 44516d2 Compare July 29, 2024 13:34

phryneas added 2 commits July 29, 2024 16:01

vitest is a valid word, so is vite

bea8025

generate all entrypoints

3f74ec5

phryneas commented Jul 29, 2024

View reviewed changes

phryneas marked this pull request as ready for review July 29, 2024 14:13

phryneas requested a review from yaacovCR August 23, 2024 15:53

phryneas mentioned this pull request Sep 10, 2024

instanceOf: disable dev instanceOf checks if NODE_ENV not set #4188

Open

yaacovCR reviewed Sep 11, 2024

View reviewed changes

yaacovCR mentioned this pull request Sep 27, 2024

v17 Roadmap #4205

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement bundling changes suggested in #4062 #4096

implement bundling changes suggested in #4062 #4096

phryneas commented May 24, 2024 •

edited

Loading

phryneas commented May 24, 2024

phryneas commented May 27, 2024

phryneas commented May 27, 2024 •

edited

Loading

phryneas commented May 27, 2024

phryneas commented May 27, 2024 •

edited

Loading

phryneas May 28, 2024

yaacovCR Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

yaacovCR Sep 11, 2024

phryneas May 28, 2024

phryneas Jul 29, 2024 •

edited

Loading

yaacovCR Sep 11, 2024

phryneas Jul 29, 2024

yaacovCR Sep 11, 2024

phryneas Sep 11, 2024

yaacovCR left a comment

yaacovCR Sep 11, 2024

yaacovCR Sep 11, 2024

phryneas commented Sep 11, 2024 •

edited

Loading

yaacovCR commented Sep 11, 2024

benjie commented Sep 18, 2024

yaacovCR commented Sep 24, 2024

yaacovCR commented Sep 25, 2024

yaacovCR commented Nov 27, 2024

phryneas commented Nov 27, 2024

implement bundling changes suggested in #4062 #4096

Are you sure you want to change the base?

implement bundling changes suggested in #4062 #4096

Conversation

phryneas commented May 24, 2024 • edited Loading

phryneas commented May 24, 2024

phryneas commented May 27, 2024

phryneas commented May 27, 2024 • edited Loading

phryneas commented May 27, 2024

phryneas commented May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacovCR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Sep 11, 2024 • edited Loading

yaacovCR commented Sep 11, 2024

benjie commented Sep 18, 2024

yaacovCR commented Sep 24, 2024

yaacovCR commented Sep 25, 2024

yaacovCR commented Nov 27, 2024

phryneas commented Nov 27, 2024

phryneas commented May 24, 2024 •

edited

Loading

phryneas commented May 27, 2024 •

edited

Loading

phryneas commented May 27, 2024 •

edited

Loading

phryneas Jul 29, 2024 •

edited

Loading

phryneas commented Sep 11, 2024 •

edited

Loading