Skip to content
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

doc: add N-API version 6 to table #32829

Closed
wants to merge 1 commit into from
Closed

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Apr 13, 2020

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), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Apr 13, 2020
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.
Copy link
Member

@jasnell jasnell left a 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 |
Copy link
Member

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).

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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
Copy link
Member Author

@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.

mhdawson added a commit that referenced this pull request Apr 17, 2020
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 <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@mhdawson
Copy link
Member Author

Landed in 2abec12

@mhdawson mhdawson closed this Apr 17, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
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 <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
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 <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 30, 2020
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 <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
@mhdawson mhdawson deleted the napi-table branch September 14, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants