Build: add support for native TypeScript#28879
Conversation
jsnajdr
requested review from
ajitbohra,
gziolo,
nerrad,
ntwb and
oandregal
as code owners
gziolo
added
[Tool] Babel preset
| ) | ||
| ) | ||
| if ( ! path || ! code ) { | ||
| return null; |
There was a problem hiding this comment.
This changes the output of engine for consumers. We should either maintain the output or adapt https://github.com/WordPress/gutenberg/blob/master/packages/docgen/src/index.js#L54 to work with nulls.
There was a problem hiding this comment.
OK, I reverted the return value of processFile to always be an object. In case of error, it will return { ast: null, tokens: null, ir: [] }. That's what the consumers and also what the tests expect.
|
|
||
| const getAST = ( source ) => { | ||
| return babel.parse( source || '' ).program; | ||
| const getAST = ( source, filename ) => { |
There was a problem hiding this comment.
Here we're not using the @babel/parser module directly, but indirectly through the @babel/core module which exports a parse function. @babel/core loads the Babel config will all the plugins and presets, and then exposes a configured parser.
This parse method is documented here and the filename option is documented here.
If the filename option is not present, the parsing will fail with error:
Error: [BABEL] unknown: Preset /* your preset */ requires a filename to be set when babel is called directly,
babel.transform(code, { filename: 'file.ts', presets: [/* your preset */] });See https://babeljs.io/docs/en/options#filename for more information.
at validateIfOptionNeedsFilename (/Users/jsnajdr/src/gutenberg/node_modules/@babel/core/lib/config/full.js:286:11)
|
Changes to I have a larger question about this PR: does it make sense to process TSX files with cc @saramarcondes as per your work on #28615 related to this as well. |
|
Agreeing with you, @nosolosw, that that should be the general goal; however, this might be a fine stop-gap for now, to at least have parameters documented without their types. I still need to sit down and figure out how TSDoc works and how we might be able to deploy its usage in the Gutenberg repository. I'll do that today and get back to y'all on whether it's something simple we could include in this PR if it's complicated enough to need a new PR. |
I may be missing some context, as I've only looked at the first file we're introducing. It doesn't look like the exported entities in the typescript files are going to have any more info than their description. So it's not that the API docs won't have the types, but that they won't have anything about input and output data (param names, types, or description and return type and description) ― there are no JSDoc My point is that it should be fine that certain typescript packages or files don't use |
Ah, I hadn't looked at the TypeScript files but you're right, that will be an issue. There's nothing preventing us from documenting those arguments, however. We can't add the types but we can add But if you think we should just hand-write those documentations then I say go for it, if the types are that important to the docs. An update on TSDoc, the main project that seems to exist for extracting TSDoc comments into Markdown is In any case, I think what you said is correct @nosolosw: we either hand write the documentation or sacrifice the types (in which case we'd need to update the code comments to include parameter and return descriptions). |
|
The only thing that JSDoc comments in TypeScript sources don't have are the types, because they are defined in the source and specifying them in JSDoc would be duplication. I can imagine the Processing this: /**
* Sum two numbers.
*
* @param a The first number
* @param b The second number
* @returns Their sum
*/
function sum( a: number, b: number ): number {
return a + b;
}Is only slightly more difficult than processing this classic JSDoc: /**
* Sum two numbers.
*
* @param {number} a The first number
* @param {number} b The second number
* @returns {number} Their sum
*/
function sum( a, b ) {
return a + b;
}The question is what to do when the types are not as neat as above. For example, we didn't have to specify the return type of |
|
@jsnajdr I spent a good chunk of time one weekend trying to re-write docgen using the TypeScript parser, which would make that possible, but it's a big endeavor, it's not a simple change. I doubt the babel parser preserves the types in the AST, so how would we extract it without converting to the TypeScript parser? |
The Babel parser can parse both TypeScript and Flow type annotations and add them to the AST. Example from astexplorer.net: |
That's incredible good news! Is that something we should implement as part of this PR? I'm happy to help with that if you'd like. |
It's definitely something that we should implement, but I believe it's not a good topic for this PR 🙂 This PR is already a spinoff from something larger (#28465) and is supposed to be reviewed and merged quickly. Let's work on this either in the #28465 branch, or in a completely new one. |
|
If the only limitation for now is that you need to duplicate TS types in JSDoc comments then we should land it as is. We don't use TS at scale so it falls in the same bucket as using ESLint disable for complex TS types in JSDoc comments. It's inconvenient but it requires more work to sort it out. Let's keep those PR small and open an issue that tracks next steps. |
jsnajdr
force-pushed
the
add/typescript-build-support
branch
from
6344596 to
2a1e369
Compare
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
@nosolosw I added some
The I believe I addressed all your review comments. Can you have another look? |
It's a bug that @sarayourfriend has addressed in another PR that should land soon. |
|
The changes related to generating API docs are fine 👍 I'm not deeply familiar with the build process, so rather let other people comment on those. |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Let's give it a try and iterate 😃
4 participants
Adds support for compiling native
.tsand.tsxsyntax to build scripts. Spinoff from #28465 that uses this tooling to implement areact-i18npackage written as native TypeScript source.First of all, we're adding the
@babel/preset-typescriptpreset to thebabel-preset-defaultso that the Gutenberg Babel config can parse the TypeScript syntax. With this support, Babel will treat the type annotations as comments and will ignore them, processing the file as standard JavaScript.The TypeScript preset adds one new constraint to how we use Babel: we need to pass the
filenamenow, in addition to the source string. Both when calling the Babel parser and when calling the Babel transformer. The TypeScript preset uses thefilenameto determine whether it should activate the TypeScript mode: it does so only for.tsx?files.When called by webpack or similar tools, Babel gets the
filenameargument. In the Gutenberg codebase, we need to add it to unit tests and also todocgenwhich uses the Babel parser.Next, we add support for
.tsx?files to the build scripts that search for source files to transpile and do the actual transpilation.Last, we need to update the
update-api-docs.jsscript so that when it encounters an API docs token in aREADME.mdfile:it finds the
src/index.tsas the package's default source file. Until now, it was looking only forsrc/index.js.