[labs/analyzer] More analysis coverage for gen-manifest #3529
Conversation
🦋 Changeset detectedLatest commit: 416e1f4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummary⏳ Benchmarks are currently running. Results below are out of date. nop-update
render
update
Results⏳ Benchmarks are currently running. Results below are out of date. ⏱ lit-element-list
render
update
update-reflect
⏱ lit-html-kitchen-sink
render
update
nop-update
⏱ lit-html-repeat
render
update
⏱ lit-html-template-heavy
render
update
⏱ reactive-element-list
render
update
update-reflect
|
|
I noticed that some of the |
| } | ||
| const {name, type, description, summary} = nameDescSummary.groups!; | ||
| const info: NamedTypedJSDocInfo = {name, type}; | ||
| if (summary !== undefined) { |
There was a problem hiding this comment.
Yes, if the regex does't find a type string in the {...}, it just goes in as undefined
| * * @slot name: summary | ||
| * * @slot name - summary | ||
| * * @slot name description | ||
| * * @slot name: description |
There was a problem hiding this comment.
Why is a : separator supported here but not in parseNamedTypedJSDocInfo?
There was a problem hiding this comment.
After consulting with Justin, we removed supporting : as a separator altogether. My best guess is I misinterpreted a regex originally implemented by Justin as intending to include : and started supporting that everywhere.
Also fixes accidentally skipped tests re: summary and removes more unused code.
| @@ -4,14 +4,21 @@ import * as path from 'path'; | |||
| import {DiagnosticsError} from './errors.js'; | |||
| import {Analyzer} from './analyzer.js'; | |||
|
|
|||
| export interface AnalyzerOptions { | |||
| exclude?: string[]; | |||
There was a problem hiding this comment.
Add comment. Are these globs?
There was a problem hiding this comment.
Done.
Yeah, globs. Useful for excluding things source like test folders that might otherwise be included in a project's tsconfig.
It might be interesting (later on, not now) to analyze a package.json's files or exports and filter down the input source to analyze based on that.
However ^^ highlights an unsolved problem in the analyzer right now (will likely need to be resolved via config) which is that just because we can determine that src/foo.ts gets output by tsc to build/foo.js doesn't mean that's where the file is ultimately importable from, since additional transforms outside of ts can (and often do) further process that into another location that is ultimately published (i.e. we have tsc output to ./development/*.js and process that further with rollup to ./*.js).
This is something we're going to hit pretty quickly when we start testing against real-world packages. It's been on my radar, but went ahead and added #3578 to track it.
There was a problem hiding this comment.
I think there are a few things we can do here that won't cover every case, but will cover a lot of them... I'll comment on the issue.
The biggest thing that clicked for me here is that these augment the tsconfig exclude setting.
| @@ -55,6 +65,7 @@ export const createPackageAnalyzer = (packagePath: AbsolutePath) => { | |||
| moduleResolution: 'node', | |||
| }, | |||
| include: ['**/*.js'], | |||
| exclude: options.exclude ? [...options.exclude] : [], | |||
There was a problem hiding this comment.
why do you need to make a copy? Does parseJsonConfigFileContent modify the array?
There was a problem hiding this comment.
Changed to options.exclude ?? []. Was probably just being paranoid I guess?
| * - If the first statement has more than one JSDoc block, we collect all | ||
| * but the last and use those, regardless of whether they contain one | ||
| * of the above module-designating tags (the last one is assumed to belong | ||
| * to the first statement). |
| @@ -406,11 +485,24 @@ export interface LitElementExport extends LitElementDeclaration { | |||
| tagname: string; | |||
| } | |||
|
|
|||
| export interface ReactiveProperty { | |||
| export interface PropertyLike extends NodeJSDocInfo { | |||
There was a problem hiding this comment.
this heritage seems a little weird to me at first read. Is JSDocInfo the right name? It seems to imply something specifically analyzed from JSDoc, but properties won't always have JSDoc comments and the main thing is that they're described - we just happened to get that info from JSDoc. So maybe name them Described, Named, etc?
There was a problem hiding this comment.
Fair, I mean those things can't be inferred from source other than from JSDoc comments, which is how I got to that naming, but I renamed them to be more generic as suggested.
| this.inheritedFrom = init.inheritedFrom; | ||
| this.source = init.source; | ||
| this.type = init.type; | ||
| this.default = init.default; |
There was a problem hiding this comment.
🏌️♂️ I once went looking for a more concise pattern for this, and this is the best I could come up with:
({
static: this.static,
privacy: this.privacy,
inheritedFrom: this.inheritedFrom,
source: this.source,
type: this.type,
default: this.default,
} = init);It removes the repeated init., but I'm not sure it's shorter enough to matter, and some people might find it weird.
| v !== undefined && | ||
| (typeof v !== 'boolean' || v === true) && | ||
| (typeof v !== 'string' || v.length > 0) && | ||
| (!Array.isArray(v) || v.length); |
There was a problem hiding this comment.
This logic was a little hard for me to parse. What about writing the positive cases?
| v !== undefined && | |
| (typeof v !== 'boolean' || v === true) && | |
| (typeof v !== 'string' || v.length > 0) && | |
| (!Array.isArray(v) || v.length); | |
| v === true || ((typeof v === 'string' || Array.isArray(v)) && v.length > 0); |
and add a comment on why true is considered not empty? I don't see that case in the expressions you replaced with the function call.
| return { | ||
| ...pickIfNotEmpty(info, 'description'), | ||
| ...pickIfNotEmpty(info, 'summary'), | ||
| ...pickIfNotEmpty(info, 'deprecated'), | ||
| }; |
There was a problem hiding this comment.
undefined properties aren't written out with JSON.stringify(), so might it be nicer to not use spread to write a single value, and do something like:
| return { | |
| ...pickIfNotEmpty(info, 'description'), | |
| ...pickIfNotEmpty(info, 'summary'), | |
| ...pickIfNotEmpty(info, 'deprecated'), | |
| }; | |
| return { | |
| description: removeEmpty(info.description), | |
| summary: removeEmpty(info.summary), | |
| deprecated: removeEmpty(info.deprecated), | |
| }; |
Or, are the all the properties in NodeJSDocInfo always going to be used? Could you transform them all?
| return { | |
| ...pickIfNotEmpty(info, 'description'), | |
| ...pickIfNotEmpty(info, 'summary'), | |
| ...pickIfNotEmpty(info, 'deprecated'), | |
| }; | |
| return removeEmpties(info); |
There was a problem hiding this comment.
undefined properties aren't written out with JSON.stringify()
OMG! 🤦 Yeah I think I knew this but totally forgot about it... definitely better to leave the key and avoid the spread. Will refactor.
There was a problem hiding this comment.
Refactored into
ifNotEmpty(v)transformIfNotEmpty(v, transform)
which always return a meaningful value or undefined, and are assigned directly to keys (not spread)
| // // TODO: ClassField | ||
| // // TODO: ClassMethod | ||
| // ], | ||
| ...useIfNotEmpty('members', [ |
There was a problem hiding this comment.
Similar comment about undefined being ok here, so we don't need spread.
| return {}; | ||
| } | ||
| const jsDocs = firstChild.getChildren().filter(ts.isJSDoc); | ||
| const moduleJSDocs = jsDocs.slice(0, jsDocs.length > 1 ? -1 : 1); |
There was a problem hiding this comment.
Might be worth some comments in here
Co-authored-by: Justin Fagnani <justinfagnani@google.com>
| v === undefined || | ||
| (typeof v === 'boolean' && v === false) || | ||
| (typeof v === 'string' && v.length === 0) || | ||
| (Array.isArray(v) && v.length === 0) |
There was a problem hiding this comment.
This can still be simplified, ie with === the typeof v === 'boolean' isn't necessary, and the undefined check isn't needed:
| v === undefined || | |
| (typeof v === 'boolean' && v === false) || | |
| (typeof v === 'string' && v.length === 0) || | |
| (Array.isArray(v) && v.length === 0) | |
| v === false || ((typeof v === 'string' || Array.isArray(v)) && v.length === 0) |
This PR includes the following:
generatecommand diagnostic errors more gracefully to CLI - 8d7b0e9--excludeoptions (e.g. exclude tests): 7fac64c) - important for excluding test files from e.g. manifest or wrapper generationClassFieldandClassMethod: 3fd322c...IfNotEmpty()helpers ingen-manifestto trim down the JSON size by only including optional fields when they have with meaningful values (e.g. non-empty string, non-empty array, true boolean, etc.)This PR successfully builds the following manifest for MWC: custom-elements.json, after making the following minor MWC source changes:
menu/lib/menu.tsActionDetailinterface{SelectedDetail}type from@firesannotationAddresses much of remaining items in #2993, with exception of:
FunctionDeclarationMixinDeclaration/CustomElementMixinDeclarationAttributes(of LitElementDeclaration)ClassFields from accessors