fix(legacy-json): more identical output #526
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
==========================================
+ Coverage 79.82% 79.89% +0.06%
==========================================
Files 121 122 +1
Lines 12017 12070 +53
Branches 840 843 +3
==========================================
+ Hits 9593 9643 +50
- Misses 2421 2424 +3
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves the legacy JSON generator to produce output that more closely matches the expected format. The changes address discrepancies found in the first 7 files alphabetically during testing.
Key changes:
- Enhanced regex pattern to match methods without a module name prefix (e.g.,
init()alongsidemodule.init()) - Refactored property type assignment to preserve both the parsed type and the original property classification using a
__promotemarker - Improved stability node detection to use
findIndexinstead of assuming the first position
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/utils/parser/constants.mjs | Updated method regex to match functions with or without module/property prefixes |
| src/generators/legacy-json/utils/parseList.mjs | Changed property handling to use Object.assign with __promote marker to preserve type information |
| src/generators/legacy-json/utils/buildSection.mjs | Refactored stability parsing to use findIndex and updated addToParent to respect __promote marker |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[Draft while I finish looking for differences. Currently at: Possible TODOs for a follow-up:
|
| section.stability = Number(stabilityNode.data.index); | ||
| section.stabilityText = stabilityNode.data.description; | ||
|
|
||
| const nodeToRemove = content.children.findIndex( |
There was a problem hiding this comment.
This feels like an expensive search, we should have the node index.
There was a problem hiding this comment.
We don't have it stored anywhere
| Object.assign(section, values[0]); | ||
|
|
||
| // TODO(@avivkeller): There is probably a better way to do this. | ||
| section.__promote = 'property'; |
There was a problem hiding this comment.
When we Object.assign(section, values[0]);, we replace section.type with values[0].type (the type of the property, i.e. string, etc)
When promoting properties to their parent (moving the section into misc, properties, modules, whatever), we check the .type, if property, we move to properties. However, since the type has been overwritten, we still need to move the object to the correct parent section, properties
There was a problem hiding this comment.
I'm unfamiliar with the __promote property, where does that come from?
There was a problem hiding this comment.
My brain xD, I just added it for this purpose. It's deleted during promotion
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
SGTM! Although tests are failing!
Yes, I'm aware. I'm going to fix them once I go through each file and confirm the diff (it's a slow process...) |
|
(Once complete, this PR will resolve that issue, since this is a regression from the old tooling to our tooling) |
|
Unrelated-ish, due to how human eyes work, when looking at these (on the page) the method signature width feels smaller than the stability index one, and that's simply due to the fact that stability index component doesn't has a border with a different colour. Could we have all our stability indexes banners to also have a border in their respective accent colour? 
|
Another possible fix is simply having the border accent of the method banner to be a stronger accent colour. |
avivkeller
force-pushed
the
more-identical-legacy-json
branch
from
1ad05ee to
842c8f6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.findIndexpropertydetection now properly assigns.typewithout changing thepropertytypetomisc(via the addition of a new__promoteproperty)init()will now be matched alongsidemy_module.init())myMethod[Symbol()](props)) are now correctednapiversions are correctly treated as numbers, and notstring[]