-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: 16.x.x
Are you sure you want to change the base?
Changes from 8 commits
85ecc7a
9646d84
16fa5d5
2fa37dc
881b1bf
864015c
f93c7bf
011ce2c
f86e6d0
73adce6
44516d2
bea8025
3f74ec5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,18 @@ | |
}, | ||
"dependencies": { | ||
"graphql": "file:../graphql.tgz", | ||
"typescript-4.1": "npm:[email protected]", | ||
"typescript-4.2": "npm:[email protected]", | ||
"typescript-4.3": "npm:[email protected]", | ||
"typescript-4.4": "npm:[email protected]", | ||
"typescript-4.5": "npm:[email protected]", | ||
"typescript-4.6": "npm:[email protected]" | ||
"typescript-4.6": "npm:[email protected]", | ||
"typescript-4.7": "npm:[email protected]", | ||
"typescript-4.8": "npm:[email protected]", | ||
"typescript-4.9": "npm:[email protected]", | ||
"typescript-5.0": "npm:[email protected]", | ||
"typescript-5.1": "npm:[email protected]", | ||
"typescript-5.2": "npm:[email protected]", | ||
"typescript-5.3": "npm:[email protected]", | ||
"typescript-5.4": "npm:[email protected]" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many versions should be supported here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this has been updated in main, maybe we can rebase this PR against main? |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Labelling a function with 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
|
@@ -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( | ||
|
@@ -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!'); | ||
|
@@ -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; | ||
} |
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; |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes break TS 4.1, but that would probably be okay for a new major of
graphql
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly causes them to break TS 4.1 out of curiosity?
do we have to remove 4.3 through 4.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for 4.1, it doesn't resolve the
exports
, while all other versions do.Tons of
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Then why don’t we remove support in a separate PR so it goes in as a separate breaking change