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

implement bundling changes suggested in #4062 #4096

Open
wants to merge 13 commits into
base: 16.x.x
Choose a base branch
from
56 changes: 55 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,62 @@
"description": "A Query Language and Runtime which can target any service.",
"license": "MIT",
"private": true,
"main": "index",
"main": "index.js",
"module": "index.mjs",
"exports": {
"./execution/execute.js": {
"types": {
"import": "./execution/execute.js.d.mts",
"default": "./execution/execute.d.ts"
},
"vitest": {
"import": "./execution/execute.js.mjs",
"default": "./execution/execute.js"
},
"module": "./execution/execute.mjs",
"import": "./execution/execute.js.mjs",
"default": "./execution/execute.js"
},
"./jsutils/instanceOf.js": {
"types": {
"import": "./jsutils/instanceOf.js.d.mts",
"default": "./jsutils/instanceOf.d.ts"
},
"vitest": {
"import": "./jsutils/instanceOf.js.mjs",
"default": "./jsutils/instanceOf.js"
},
"module": "./jsutils/instanceOf.mjs",
"import": "./jsutils/instanceOf.js.mjs",
"default": "./jsutils/instanceOf.js"
},
"./language/parser.js": {
"types": {
"import": "./language/parser.js.d.mts",
"default": "./language/parser.d.ts"
},
"vitest": {
"import": "./language/parser.js.mjs",
"default": "./language/parser.js"
},
"module": "./language/parser.mjs",
"import": "./language/parser.js.mjs",
"default": "./language/parser.js"
},
"./language/ast.js": {
"types": {
"import": "./language/ast.js.d.mts",
"default": "./language/ast.d.ts"
},
"vitest": {
"import": "./language/ast.js.mjs",
"default": "./language/ast.js"
},
"module": "./language/ast.mjs",
"import": "./language/ast.js.mjs",
"default": "./language/ast.js"
}
},
"typesVersions": {
">=4.1.0": {
"*": [
Expand Down
187 changes: 187 additions & 0 deletions resources/build-npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@ const {
showDirStats,
} = require('./utils.js');

const entryPoints = fs
.readdirSync('./src', { recursive: true })
.filter((f) => f.endsWith('index.ts'))
.map((f) => f.replace(/^src/, ''))
.reverse()
.concat([
'execution/execute.ts',
'jsutils/instanceOf.ts',
'language/parser.ts',
'language/ast.ts',
Comment on lines +22 to +25
Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

]);

if (require.main === module) {
fs.rmSync('./npmDist', { recursive: true, force: true });
fs.mkdirSync('./npmDist');
Expand Down Expand Up @@ -57,11 +69,21 @@ if (require.main === module) {

const tsProgram = ts.createProgram(['src/index.ts'], tsOptions, tsHost);
const tsResult = tsProgram.emit();

assert(
!tsResult.emitSkipped,
'Fail to generate `*.d.ts` files, please run `npm run check`',
);

for (const [filename, contents] of Object.entries(
buildCjsEsmWrapper(
entryPoints.map((e) => './src/' + e),
tsProgram,
),
)) {
writeGeneratedFile(filename, contents);
}

assert(packageJSON.types === undefined, 'Unexpected "types" in package.json');
const supportedTSVersions = Object.keys(packageJSON.typesVersions);
assert(
Expand Down Expand Up @@ -107,6 +129,44 @@ function buildPackageJSON() {
delete packageJSON.scripts;
delete packageJSON.devDependencies;

packageJSON.type = 'commonjs';
yaacovCR marked this conversation as resolved.
Show resolved Hide resolved

for (const entryPoint of entryPoints) {
if (!entryPoint.endsWith('index.ts')) {
continue;
}
const base = ('./' + path.dirname(entryPoint)).replace(/\/.?$/, '');
const generated = {};
generated[base] = {
types: {
import: base + '/index.js.d.mts',
default: base + '/index.d.ts',
},
/*
this is not automatically picked up by vitest, but we can instruct users to add it to their vitest config:
```js title="vite.config.ts"
import { defineConfig } from 'vite';
export default defineConfig(async ({ mode }) => {
return {
resolve: mode === 'test' ? { conditions: ['vitest'] } : undefined,
};
});
```
*/
vitest: {
import: base + '/index.js.mjs',
default: base + '/index.js',
},
module: base + '/index.mjs',
import: base + '/index.js.mjs',
default: base + '/index.js',
};
yaacovCR marked this conversation as resolved.
Show resolved Hide resolved
packageJSON.exports = {
...generated,
...packageJSON.exports,
};
}

// TODO: move to integration tests
const publishTag = packageJSON.publishConfig?.tag;
assert(publishTag != null, 'Should have packageJSON.publishConfig defined!');
Expand Down Expand Up @@ -137,3 +197,130 @@ function buildPackageJSON() {

return packageJSON;
}

/**
*
* @param {string[]} files
* @param {ts.Program} tsProgram
* @returns
*/
function buildCjsEsmWrapper(files, tsProgram) {
/**
* @type {Record<string, string>} inputFiles
*/
const inputFiles = {};
for (const file of files) {
const sourceFile = tsProgram.getSourceFile(file);
assert(sourceFile, `No source file found for ${file}`);

const generatedFileName = path.relative(
path.dirname(tsProgram.getRootFileNames()[0]),
file.replace(/\.ts$/, '.js.mts'),
);
const exportFrom = ts.factory.createStringLiteral(
'./' + path.basename(file, '.ts') + '.js',
);

/**
* @type {ts.Statement[]}
*/
const statements = [];

/** @type {string[]} */
const exports = [];

/** @type {string[]} */
const typeExports = [];

sourceFile.forEachChild((node) => {
if (ts.isExportDeclaration(node)) {
if (node.exportClause && ts.isNamedExports(node.exportClause)) {
for (const element of node.exportClause.elements) {
if (node.isTypeOnly || element.isTypeOnly) {
typeExports.push(element.name.text);
} else {
exports.push(element.name.text);
}
}
}
} else if (
node.modifiers?.some(
(modifier) => modifier.kind === ts.SyntaxKind.ExportKeyword,
)
) {
if (ts.isVariableStatement(node)) {
for (const declaration of node.declarationList.declarations) {
if (declaration.name && ts.isIdentifier(declaration.name)) {
exports.push(declaration.name.text);
}
}
} else if (
ts.isFunctionDeclaration(node) ||
ts.isClassDeclaration(node)
) {
exports.push(node.name.text);
} else if (ts.isTypeAliasDeclaration(node)) {
typeExports.push(node.name.text);
}
}
});
if (exports.length > 0) {
statements.push(
ts.factory.createExportDeclaration(
undefined,
undefined,
false,
ts.factory.createNamedExports(
exports.map((name) =>
ts.factory.createExportSpecifier(false, undefined, name),
),
),
exportFrom,
),
);
}
if (typeExports.length > 0) {
statements.push(
ts.factory.createExportDeclaration(
undefined,
undefined,
true,
ts.factory.createNamedExports(
typeExports.map((name) =>
ts.factory.createExportSpecifier(false, undefined, name),
),
),
exportFrom,
),
);
}
const printer = ts.createPrinter({ newLine: ts.NewLineKind.LineFeed });
inputFiles[generatedFileName] = printer.printFile(
ts.factory.createSourceFile(
statements,
ts.factory.createToken(ts.SyntaxKind.EndOfFileToken),
ts.NodeFlags.None,
),
);
}
/**
* @type {ts.CompilerOptions} options
*/
const options = {
...tsProgram.getCompilerOptions(),
declaration: true,
emitDeclarationOnly: false,
isolatedModules: true,
module: ts.ModuleKind.ESNext,
};
options.outDir = options.declarationDir;
const results = {};
const host = ts.createCompilerHost(options);
host.writeFile = (fileName, contents) => (results[fileName] = contents);
host.readFile = (fileName) => inputFiles[fileName];

const program = ts.createProgram(Object.keys(inputFiles), options, host);
program.emit();

return results;
}
11 changes: 11 additions & 0 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * ExecutionContext
* * assertValidExecutionArguments @internal
* * buildExecutionContext @internal
* * buildResolveInfo @internal
* * getFieldDef @internal
* Should we still expose this file?
*/

import { devAssert } from '../jsutils/devAssert';
import { inspect } from '../jsutils/inspect';
import { invariant } from '../jsutils/invariant';
Expand Down
6 changes: 6 additions & 0 deletions src/execution/values.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`: none
* Should we still expose this file?
*/

import { inspect } from '../jsutils/inspect';
import { keyMap } from '../jsutils/keyMap';
import type { Maybe } from '../jsutils/Maybe';
Expand Down
4 changes: 4 additions & 0 deletions src/jsutils/Maybe.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
/**
* Quoted as "used by external libraries".
* Should we still expose these?
*/
/** Conveniently represents flow's "Maybe" type https://flow.org/en/docs/types/maybe/ */
export type Maybe<T> = null | undefined | T;
6 changes: 6 additions & 0 deletions src/jsutils/ObjMap.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/**
* Quoted as "used by external libraries".
* All of these could be replaced by Record<string, T> or Readonly<Record<string, T>>

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-html-tag-missing-greater-than: The HTML tag has invalid syntax: Expecting an attribute or ">" or "/>"

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-escape-greater-than: The ">" character should be escaped using a backslash to avoid confusion with an HTML tag

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-html-tag-missing-greater-than: The HTML tag has invalid syntax: Expecting an attribute or ">" or "/>"

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-html-tag-missing-greater-than: The HTML tag has invalid syntax: Expecting an attribute or ">" or "/>"

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-escape-greater-than: The ">" character should be escaped using a backslash to avoid confusion with an HTML tag

Check failure on line 3 in src/jsutils/ObjMap.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

tsdoc-escape-greater-than: The ">" character should be escaped using a backslash to avoid confusion with an HTML tag
* Should we still expose these?
*/

export interface ObjMap<T> {
[key: string]: T;
}
Expand Down
4 changes: 4 additions & 0 deletions src/jsutils/PromiseOrValue.ts
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
/**
* Quoted as "used by external libraries".
* Should we still expose these?
*/
export type PromiseOrValue<T> = Promise<T> | T;
7 changes: 7 additions & 0 deletions src/language/ast.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * isNode @internal
* * QueryDocumentKeys @internal
* Should we still expose this file?
*/
import type { Kind } from './kinds';
import type { Source } from './source';
import type { TokenKind } from './tokenKind';
Expand Down
7 changes: 7 additions & 0 deletions src/language/lexer.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * isPunctuatorTokenKind @internal
* Should we still expose this file?
*/

import { syntaxError } from '../error/syntaxError';

import { Token } from './ast';
Expand Down
7 changes: 7 additions & 0 deletions src/language/parser.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * Parser @internal
* Should we still expose this file?
*/

import type { Maybe } from '../jsutils/Maybe';

import type { GraphQLError } from '../error/GraphQLError';
Expand Down
7 changes: 7 additions & 0 deletions src/type/schema.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * GraphQLSchemaNormalizedConfig @internal
* * GraphQLSchemaValidationOptions
* Should we still expose this file?
*/
import { devAssert } from '../jsutils/devAssert';
import { inspect } from '../jsutils/inspect';
import { instanceOf } from '../jsutils/instanceOf';
Expand Down
10 changes: 10 additions & 0 deletions src/validation/ValidationContext.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * ASTValidationContext
* * ASTValidationRule
* * SDLValidationContext
* * SDLValidationRule
* Should we still expose this file?
*/

import type { Maybe } from '../jsutils/Maybe';
import type { ObjMap } from '../jsutils/ObjMap';

Expand Down
7 changes: 7 additions & 0 deletions src/validation/specifiedRules.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/**
* Quoted as "used by external libraries".
* Missing exports from `graphql`:
* * specifiedSDLRules @internal
* Should we still expose this file?
*/

// Spec Section: "Executable Definitions"
import { ExecutableDefinitionsRule } from './rules/ExecutableDefinitionsRule';
// Spec Section: "Field Selections on Objects, Interfaces, and Unions Types"
Expand Down
Loading
Loading