Docs-tools: Replace doctrine with jsdoc-type-pratt-parser#26305
Conversation
This removes doctrine and `assert` from the docs-tools package. The `type` of a parsed JSDoc comment will now be a `RootResult` from the `jsdoc-type-pratt-parser` package rather than a doctrine `Type`. Other than that, existing behaviour should remain.
| "name": "case13", | ||
| "table": { | ||
| "defaultValue": null, | ||
| "jsDocTags": { |
There was a problem hiding this comment.
this one has disappeared because the source has no name:
/**
* param with type
* @param {SyntheticEvent}
*/
case13: PropTypes.func,in the old source, this code looks like it should've caught that and returned null:
const paramName = tag.name;
// When the @param doesn't have a name but have a type and a description, "null-null" is returned.
if (paramName != null && paramName !== 'null-null') {my guess is, at some point, doctrine stopped giving us null-null as a special value? so the branch has been passing?
the new source does the same check:
if (!tag.name || tag.name === '-') {
return null;
}so i'm not sure. the same "empty name" check now succeeds so we get null correctly
shilman
added
maintenance
|
This is amazing @43081j, thanks so much !!! We are just a few days from releasing 8.0, so we're going to punt this until 8.1. It's technically a breaking change, but it's an undocumented API so I discussed with the team and we'd rather take a little more time than risk delaying or borking 8.0. Will review properly and merge it into 8.1 as soon as the dust has settled. |
shilman
changed the title
doctrine with jsdoc-type-pratt-parser
|
All good, that makes sense to me too Let me know if you need any help once you get around to that 8.1 merge |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e20977a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
ndelangen
added
ci:daily
shilman
left a comment
shilman
left a comment
There was a problem hiding this comment.
@43081j Looks like this wound up in my lap at some point but I missed it. Changes look great and I have approved. We're releasing 8.1 in the next day or two and then I will merge as soon as we get that out. Thanks for your patience on this!
|
no worries, thanks for taking a look let me know if you need any help with anything 👍 |
shilman
added
ci:daily
6 participants
This removes doctrine and
assertfrom the docs-tools package.The
typeof a parsed JSDoc comment will now be aRootResultfrom thejsdoc-type-pratt-parserpackage rather than a doctrineType.Other than that, existing behaviour should remain.
@shilman is this what you had in mind in #26276?
A question for maintainers: this will of course change the
typewe expose on anExtractedJsDocParam. There's not a lot we can do about that since it used to be doctrine'sType, and I don't think we should try map back to the old model (it isn't simple). Do you know what consumes that property? is it a breaking change?Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
None yet, as I wasn't sure the best way to manually prove out the various possible JSDoc types.
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>