-
Notifications
You must be signed in to change notification settings - Fork 134
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 minutes for meeting 17 Oct 2018 #613
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,166 @@ | ||
# Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-10-17 | ||
|
||
## Links | ||
|
||
* **Recording**: https://www.youtube.com/watch?v=oFZFNdv9Jf0 | ||
* **GitHub Issue**: https://github.com/nodejs/TSC/issues/612 | ||
|
||
## Present | ||
|
||
* Anna Henningsen @addaleax (TSC) | ||
* Anatoli Papirovski @apapirovski (TSC) | ||
* Сковорода Никита Андреевич @ChALkeR (TSC) | ||
* Colin Ihrig @cjihrig (TSC) | ||
* Gabriel Schulhof @gabrielschulhof (TSC) | ||
* Joyee Cheung @joyeecheung (TSC) | ||
* Michael Dawson @mhdawson (TSC) | ||
* Myles Borins @MylesBorins (TSC) | ||
* Ali Ijaz Sheikh @ofrobots (TSC) | ||
* Refael Ackermann @refack (Participant) | ||
* Rod Vagg @rvagg (TSC) | ||
* Rich Trott @Trott (TSC) | ||
|
||
## Agenda | ||
|
||
### Announcements | ||
|
||
*Extracted from **tsc-agenda** labeled issues and pull requests from the **nodejs org** prior to the meeting. | ||
|
||
|
||
### nodejs/node | ||
|
||
* doc: formalize non-const reference usage in C++ style guide [#23155](https://github.com/nodejs/node/pull/23155) | ||
* Rafael | ||
* refreshing knowledge of “modern” C++ | ||
* have recently turned on C++ 14 | ||
* discussion around 2 subjects, auto and non-const references | ||
* Anna mentioned that the common rule is to not use auto and non-const references | ||
* Rafael feels that is against where the language is going, for example in C++ core guidelines | ||
* Their (core guidelines) code for intent as opposed to memory arithmetic, casting, being | ||
specific about. Shifting towards more standard use of idioms and standard libraries, code | ||
for the intent not compilers. | ||
* auto tells compiler, still strongly typed, but that compiler can derive the type.Since templates | ||
already do this then its ok, does not give the compiler any new information. In the end that | ||
was discussed in a different issue, which was don’t use auto unless it is saving you a lot of | ||
time | ||
* Agreed to split discussion and discuss reference variables in more detail. But don’t have | ||
consensus on this point, hence this discussion at the meeting. | ||
* Google style guidelines say that non-const references to args don’t make sense. | ||
* Not a lot of counter arguments. | ||
* Does not believe that saying don’t use non-const references use pointers instead. | ||
* Affects our code, as code that comes in uses non-const references for example using | ||
Containers. | ||
|
||
* Anna | ||
* We pretty much follow the style that V8 uses even if it is not in the style guides | ||
* Understands where Rafael is coming from and making it easy for newcomers, there is still | ||
value in making it easy for reviewers as this makes it obvious when something is changed | ||
or not. | ||
* PR is capturing what we have been doing, but which has not yet been formalized. | ||
|
||
* Nikita, how would accepting this affect our code? | ||
|
||
* Gabriel, does it make sense to add additional reasons in the PR itself so that its clearer why | ||
the guideline is there. | ||
|
||
* Rafael | ||
* in favor of documenting as he was looking for rational/reasoning to be explained | ||
* recently we landed general guidelines which say follow core guidelines unless they | ||
conflict with google guidelines or our own. | ||
* still missing motivation to be so cut and dry about pointers being a better choice | ||
|
||
* Nikita | ||
* Do we have any code that does not follow that rule, | ||
|
||
* Rafael | ||
* yes there are many v8 APIs that required you use non-const references | ||
* we have been fairly lax in the past of enforcing style guide as it has not been well formalized | ||
in the past | ||
|
||
* Joyee | ||
* In some ways our C++ is basically V8 C++, because it has been based on using | ||
the V8 APIs | ||
* Style should be aligned with V8 style because of this dependency. | ||
* Most of non-const references are overloads, most of the time they still use pointers | ||
* We could probably list out specific examples as opposed to saying pointers are always | ||
Preferred. | ||
* Rich kick back to github | ||
|
||
* Nikita | ||
* If we relax the rule (based on it being defacto), what can we change in our existing | ||
code base that would improve it? | ||
|
||
* Rafael | ||
* use of for in ?over container as opposed to having to doing bounds checking. | ||
* for each in newer compilers | ||
* avoids needs for null checks in some cases | ||
* Don’t believe it will be very different from V8, but don’t agree completely we should | ||
infer what we think the v8 team was trying to get at. | ||
|
||
* Anna | ||
* One of main arguments, non const refs indicate that API is going to change something, | ||
but the same is true for pointers as well. | ||
|
||
* Ali propose | ||
* Document what we have now (existing practice) | ||
* Have a new issue/PR to change it if we want to agree to do something different. | ||
|
||
* Take it back to GitHub. | ||
|
||
* Rafael, high barrier to entry for our C++ code, small number of people maintaining. Would | ||
like to have larger group participating. | ||
|
||
* fs: behaviour of readFile and writeFile with file descriptors [#23433](https://github.com/nodejs/node/issues/23433) | ||
* please review, added by thefourtheye for which time is bad, so we’ll discuss next week | ||
|
||
### nodejs/TSC | ||
|
||
* Tracking issue for updating TSC on Board Meetings [#476](https://github.com/nodejs/TSC/issues/476) | ||
* Short board meeting last week, mostly going through the numbers from the conference | ||
* just under 1000 tickets sold ~20% growth in terms of attendance and funds raised | ||
* Announced “intent” to merge, had open town hall | ||
* Invited to join discussion in nodejs/bootstrap - https://github.com/nodejs/bootstrap/issues/10 | ||
|
||
* Strategic Initiatives - Tracking Issue [#423](https://github.com/nodejs/TSC/issues/423) | ||
* Modules | ||
* Minimal kernel landed 2 weeks ago in fork | ||
* Can import es modules with .mjs, most of other contentious features are removed. | ||
* Currently looking at uncontentious user improvements | ||
* Working on module loaders | ||
* Week of the 31st should have more updates | ||
* N-API | ||
* working on NodeConfEU workshop | ||
* OpenSSL | ||
* No updates, we’ll have to get 1.1.1 in during LTS | ||
* Workers | ||
* No update | ||
* Error Messages | ||
* Nothing this week | ||
* Governance | ||
* covered mostly in board related stuff. | ||
* Good session at collaborator summit, follow work in bootstrap repo. | ||
* Focus possibly shifted to that of combined foundation | ||
* Async Hooks | ||
* Nothing substantial to add | ||
* Discussion around async context formalization | ||
* may suggest a different model, worth reviewing if you have an interest in this. | ||
* Open web standards | ||
* Myles, Joyee would you like to become the champion. | ||
* Joyee, spend quite some time on the process to propose to implement | ||
new web feature in core. Lots of suggestions and thoughts on what should be | ||
included in the proposal. | ||
* Some good brainstorming of the APIs we might want to include | ||
* Also discussion of how we might want to participate. | ||
* Minutes will be PR’d into summit agenda, issue #106 | ||
|
||
## Q&A, Other | ||
|
||
* No questions this week. | ||
|
||
## Upcoming Meetings | ||
|
||
* **Node.js Foundation Calendar**: https://nodejs.org/calendar | ||
|
||
Click `+GoogleCalendar` at the bottom right to add to your own Google calendar. | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Sorry about this. I tried hard but couldn't stay awake.
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.
we can't expect people to stay up in the middle of the night!