Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
doc: add N-API version 6 to table#32829
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 our terms of service and privacy 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
mhdawson commented Apr 13, 2020 • 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.
We missed adding version 6 to the compatibility table when we defined version 6. Add it along with the versions that we know will include version 6.
jasnell 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.
My one concern is the scalability of this table as new levels are added. Might be worth refactoring to avoid the table growing too wide?
| | v11.x | v11.0.0 | v11.0.0 | v11.0.0 | v11.8.0 | | | | ||
| | v12.x | v12.0.0 | v12.0.0 | v12.0.0 | v12.0.0 | v12.11.0 | | | ||
| | v13.x | v13.0.0 | v13.0.0 | v13.0.0 | v13.0.0 | v13.0.0 | | | ||
| | v14.x | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | |
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.
The usual process is to add REPLACEME placeholders that get replaced in the release commit (i.e. 14.0.0).
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.
@richardlau will that do the right thing when it gets backported?
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.
I think that depends on whether the commit is picked from current or master.
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.
Since this is a matrix adding REPLACEME for all versions will not do what we want. Maybe we should add REPLACEME for the version corresponding to the branch against which the commit is being applied, and hardcoded version values for all the other branches.
This means that when we backport this commit we'll necessarily have to edit it.
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.
I'm happy to break from convention/the usual process if it makes sense and does the right thing. I make a point of pointing out when we do diverge from the norm though so that we do so consciously.
mhdawson commented Apr 15, 2020
@jasnell will think about how to refactor, but I'd like to land as is, as version 6 is already out and would like to avoid confusion in the short term. |
We missed adding version 6 to the compatibility table when we defined version 6. Add it along with the versions that we know will include version 6. PR-URL: #32829 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
mhdawson commented Apr 17, 2020
Landed in 2abec12 |
We missed adding version 6 to the compatibility table when we defined version 6. Add it along with the versions that we know will include version 6. PR-URL: #32829 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
We missed adding version 6 to the compatibility table when we defined version 6. Add it along with the versions that we know will include version 6. PR-URL: #32829 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
We missed adding version 6 to the compatibility table when we defined version 6. Add it along with the versions that we know will include version 6. PR-URL: #32829 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
We missed adding version 6 to the compatibility
table when we defined version 6. Add it along with the
versions that we know will include version 6.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes