Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(ast-spec): correct some incorrect ast types#6257
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. |
nx-cloudbot commentedDec 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
netlifybot commentedDec 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
codecovbot commentedDec 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## main #6257 +/- ##==========================================- Coverage 91.28% 91.27% -0.02%========================================== Files 366 366 Lines 12445 12445 Branches 3645 3645 ==========================================- Hits 11361 11359 -2+ Misses 773 772 -1- Partials 311 314 +3
Flags with carried forward coverage won't be shown.Click here to find out more.
|
| importtype{SpreadElement}from'../element/SpreadElement/spec'; | ||
| exporttypeObjectLiteralElement=MethodDefinition|Property|SpreadElement; | ||
| exporttypeObjectLiteralElement=Property|SpreadElement; |
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.
Huh, I guess they're not the same in objects. Cool.
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.
Yeah objects useProperty withmethod: true
There's definitely scope to work on theProperty type to better discriminate the union based on the flags.
I might raise a PR to do that soon as it's a pretty nice property to have hyper-specific types for this sort of case.
I actually did it when I wrote the flow-estree types
https://github.com/facebook/hermes/blob/main/tools/hermes-parser/js/hermes-estree/src/types.js#L448-L536
(looks like I didn't do method types, but that's a natural next step)
JoshuaKGoldberg left a comment
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.
If you're confident changing the types here isn't a breaking change, then I am! LGTM :) thanks for doing these.
bradzacher commentedDec 21, 2022
For posterity - in theory these are breaking changes as they will cause errors in consumer's code. In practice however we consider this a bug fix because the types did not match reality - the existing runtime values did not match the old types - meaning any new type errors in consuming are flagging code that may be broken and may crash for some cases. |
Josh-Cena commentedDec 21, 2022
@bradzacher Sorry for the late message, but as a heads-up, virtually all expressions' type defs are too narrow after removal of the |
* fix(ast-spec): correct some ast types* type error
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.47.0` -> `5.47.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.47.0/5.47.1) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.47.0` -> `5.47.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.47.0/5.47.1) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v5.47.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5471-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5470v5471-2022-12-26)[Compare Source](typescript-eslint/typescript-eslint@v5.47.0...v5.47.1)##### Bug Fixes- **ast-spec:** correct some incorrect ast types ([#​6257](typescript-eslint/typescript-eslint#6257)) ([0f3f645](typescript-eslint/typescript-eslint@0f3f645))- **eslint-plugin:** \[member-ordering] correctly invert optionalityOrder ([#​6256](typescript-eslint/typescript-eslint#6256)) ([ccd45d4](typescript-eslint/typescript-eslint@ccd45d4))</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v5.47.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5471-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5470v5471-2022-12-26)[Compare Source](typescript-eslint/typescript-eslint@v5.47.0...v5.47.1)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43My4xIiwidXBkYXRlZEluVmVyIjoiMzQuNzMuMSJ9-->Co-authored-by: cabr2-bot <cabr2.help@gmail.com>Reviewed-on:https://codeberg.org/Calciumdibromid/CaBr2/pulls/1693Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
PR Checklist
Overview
These changes will be disruptive to downstream consumers but they are correct and will help expose potentially bad code.
Particularly the sparse array type fix because it's meant to include
null, not just some other node type. They're a rarer case in code (because sparse arrays are one of the dumbest features in JS), but if encountered it will crash code that doesn't expect it.