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

Add support info for Web APIs that node supports #3160

Merged
merged 16 commits into from
Oct 30, 2019

Conversation

freaktechnik
Copy link
Contributor

@freaktechnik freaktechnik commented Nov 30, 2018

Node supports a couple of web APIs out of the box:

  • URL is implemented to follow the whatwg spec
  • URLQueryParams is implemented to follow the whatwg spec
  • console follows the console spec too, afaik
  • setInterval/setTimeout are very close to their web equivalent

I am aware that currently this information is not shown on MDN at all, since node is not enabled to be rendered in api/


Table of all the APIs and corresponding node docs:

API Node documentation
URL https://nodejs.org/api/url.html#url_the_whatwg_url_api and https://nodejs.org/api/globals.html#globals_url
URLSearchParams https://nodejs.org/api/url.html#url_class_urlsearchparams and https://nodejs.org/api/globals.html#globals_urlsearchparams
console https://nodejs.org/api/console.html and https://nodejs.org/api/globals.html#globals_console
setInterval https://nodejs.org/api/timers.html#timers_setinterval_callback_delay_args and https://nodejs.org/api/globals.html#globals_setinterval_callback_delay_args
setTimeout https://nodejs.org/api/timers.html#timers_settimeout_callback_delay_args and https://nodejs.org/api/globals.html#globals_settimeout_callback_delay_args
clearInterval https://nodejs.org/api/timers.html#timers_clearinterval_timeout and https://nodejs.org/api/globals.html#globals_clearinterval_intervalobject
clearTimeout https://nodejs.org/api/timers.html#timers_cleartimeout_timeout and https://nodejs.org/api/globals.html#globals_cleartimeout_timeoutobject
setImmediate https://nodejs.org/api/timers.html#timers_setimmediate_callback_args and https://nodejs.org/api/globals.html#globals_setimmediate_callback_args
clearImmediate https://nodejs.org/api/timers.html#timers_clearimmediate_immediate and https://nodejs.org/api/globals.html#globals_clearimmediate_immediateobject
TextDecoder https://nodejs.org/api/util.html#util_class_util_textdecoder and https://nodejs.org/api/globals.html#globals_textdecoder
TextEncoder https://nodejs.org/api/util.html#util_class_util_textencoder and https://nodejs.org/api/globals.html#globals_textencoder
Performance API (not listing those out) https://nodejs.org/api/perf_hooks.html
Worker, Message* https://nodejs.org/api/worker_threads.html#worker_threads_class_messagechannel and onward

@Elchi3 Elchi3 added data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:browsers 🌍 Data about browsers (versions, release dates, etc). This data is used for validation. labels Dec 3, 2018
@Elchi3
Copy link
Member

Elchi3 commented Dec 3, 2018

Hm, this is tricky, but a good point. I will think about APIs and nodejs implementation.

You've added new nodejs versions, we need to double check them if these were really the ones when support was introduced. So, if you have links, bugs or any other first hand material, it would ease the review. Thanks!

@freaktechnik
Copy link
Contributor Author

freaktechnik commented Dec 3, 2018

Sure, those versions are all from the node documentation:

The version a feature was introduced in is usually in small text below the heading for it. If there were changes in other versions it is a table that can be shown by clicking "History".

@freaktechnik
Copy link
Contributor Author

I've used true for when the subfeature was introduced with the main feature or in the case of timers to avoid saying it was added in "0.0.1" and instead just say it has always been there.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 6, 2018

In this case, we could modify the {{Compat}} macro to test if nodejs is present in any of the queried __compat keys for the api namespace.

Also, how about docuenting support for JSDOM (on npm) (issue #3180)?

@freaktechnik
Copy link
Contributor Author

freaktechnik commented Dec 21, 2018

I've added info on TextDecoder and Encoder, since those have actually been supported by node for quite some time too and were recently added to the global context.

See

@freaktechnik
Copy link
Contributor Author

Found another API that node supports: https://nodejs.org/api/perf_hooks.html

@ddbeck
Copy link
Collaborator

ddbeck commented Apr 22, 2019

Hm, this is tricky, but a good point. I will think about APIs and nodejs implementation.

@Elchi3 have you given any more thought to including Node.js's equivalents to web APIs in BCD?

@Elchi3
Copy link
Member

Elchi3 commented Apr 23, 2019

So, I think there are a few things that we want to do here:

  1. Review the data updates in this PR.
  2. Review the updates to browsers/nodejs.json https://github.com/mdn/browser-compat-data/pull/3160/files#diff-2f2da23b76b41b4b8f74813e5959cfd8 Does it really make sense for us to have "0.1.100" and similar as releases? (we normally only allow major releases, that is releases that introduced feature changes).
  3. Follow-up with a renderer issue or PR (kumascript), so that we change the logic so that nodejs is displayed if we have non-null data (don't always list nodejs there, as the table is already long enough, and I don't think it makes sense to display node for all web api data)

@freaktechnik
Copy link
Contributor Author

freaktechnik commented Apr 23, 2019

I just noticed I had pushed it and noted it in the commit but not explicitly written a comment on it: I've added node's experimental Worker API as entry in the DOM worker API. It has some similarities, but it is a very different concept and I'm not really convinced it warrants inclusion. I mainly added the commit to put it on the table.

Does it really make sense for us to have "0.1.100" and similar as releases? (we normally only allow major releases, that is releases that introduced feature changes).

Those versions are directly from the versions noted in the official docs as to when this has become available. It is worthy to note that node didn't really do proper major releases prior to their re-merging with IO, which is v4, iirc. In semver 0.1.11 -> 0.1.12 is a "minor" bump (or as it's now coined an enhancement bump or whatever). They also often add new APIs in semver "minor" versions (so the second number, not the first one).

As is noted further up I sometimes just used "version_added": true when an API was added in 0.0.1 or similar. I also used true in cases where you maybe don't want to use it anymore (when subfeatures were added in the same version as the parent feature). Happy to update those.

@queengooborg
Copy link
Collaborator

I think that historically, we haven't kept pre-1.0 releases for browsers, and I feel that Node.js should be no different. Anything that's before 1.0, we should bump up to 1.0 (not set as true, since that would mean we don't know when they were added, which we do).

@ddbeck
Copy link
Collaborator

ddbeck commented Apr 24, 2019

@vinyldarkscratch I don't think the no-pre-1.0 approach will make sense for Node given its release and governance history. The short version is that Node.js didn't use major version release numbers in its early history; io.js forked Node and started versioning from 1.0; io.js and Node merged back together again, versioning from 4.0. Ignoring pre-0.10 Node releases is probably more like what we do for browsers, but I don't think it's strictly required for us to do that, particularly if we've got reliable data for those releases. I think the no-pre-1.0 approach is more of a pragmatic thing, about finding reliable data.

@queengooborg
Copy link
Collaborator

I see what you mean. Alright, if that's the case, I'm all for having 0.x releases in the data!

@Elchi3
Copy link
Member

Elchi3 commented May 2, 2019

I think I'm also +1 on adding the 0.x releases

  • "0.1.100"
  • "0.1.101"
  • "0.1.104"

However, this also adds a few more releases and I want to shed some light we haven't added them so far.
For our nodejs data, the https://node.green/ site has been a helpful resource for getting nodejs data. At the top of the site, you can see a row with release versions. Upon hovering it, you can see a list of browsers with identical results, because they all share the same v8 version (or sometimes just minor v8 upgrades).

So, when you have "7.5.0" on node.green, and you hover it, you can actually see it goes back to "7.0.0", which is the first release where this v8 version was used.
This is also the reason why, so far, we've only allowed use "7.0.0" and not "7.5.0", because when people think it is in "7.5.0" we should actually put "7.0.0" as it was already supported back then given it is the same v8 version. (remember we use "version_added", so "7.0.0" makes more sense).

But this is only true for ECMAScript features (I believe as this is what this PR suggests). See for example #3970 where this is the case and I asked to use a prior version.

As this PR deals with non-ECMAScript features for node, new version numbers come in the game:

  • "7.5.0" (would be 7.0.0 for ES features)
  • "7.7.0" (would be 7.6.0 for ES features)
  • "7.10.0" (would be 7.6.0 for ES features)
  • "9.3.0" (would be 7.6.0 for ES features)
  • "10.5.0" (would be 10.4.0 for ES features)
  • "10.7.0" (would be 10.4.0 for ES features)
  • "11.7.0" (would be 11.0.0 for ES features)

If we would add "7.5.0" (and others, see above) to the data, I think it should still be invalid for ECMAScript features so that always use the first version when a new v8 engine version was used.

Am I making sense?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented May 2, 2019

For that, the v8 engine version would be equal.

I could add additional handling to test‑versions for that.

@queengooborg
Copy link
Collaborator

queengooborg commented May 10, 2019

Ah whoops, sorry about the bad merge, @freaktechnik! (Usually we don't do the merging, but I got bored, haha)

@Elchi3
Copy link
Member

Elchi3 commented Jun 3, 2019

So, in order to move forward with adding nodejs version to non-ECMAScript features, I would like to move the addition of more nodejs versions in browsers/nodejs.json into a separate PR that will be a prerequisite for this PR. In that PR, we should also write a test in test-versions.js that makes sure we're not using these additional nodejs versions for features in the javascript/ folder (as suggested in #3160 (comment)).

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jun 7, 2019

I’ve now implemented the test in #4264.

@freaktechnik
Copy link
Contributor Author

Thank you, I'll try to rebase/merge once that's merged.

@queengooborg
Copy link
Collaborator

Thanks for rebasing! Looking at this PR again, it seems that it's gotten fairly larger. To help speed up the review process, we may want to consider cherry-picking changes into separate PRs to keep the scope of each as small as possible. At that point, we can then gradually add the data in, one piece at a time. Does that sound reasonable to you?

so I may have to add null (to avoid nodejs being shown?) to most of the APIs in those two files?

null will still show NodeJS in the compatibility tables, and leaving the browser out of the data implies null. It will render as a question mark, saying "we don't know the support for this", thus inviting anyone who does know to submit a PR later on. 😉

@queengooborg
Copy link
Collaborator

Hey @freaktechnik, do you plan to return to this PR?

@freaktechnik
Copy link
Contributor Author

freaktechnik commented Oct 24, 2019

Sure, however so far I've only seen hypotheticals for possible changes and no word from a potential reviewer, so I've seen no reason to take action.

@queengooborg
Copy link
Collaborator

Well, I was sort of hoping that we could start cherry-picking groups of changes so we can get them merged a little quicker?

@Elchi3 Elchi3 self-requested a review October 28, 2019 15:50
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience and I'm sorry it took so long to get to this. As you know, this won't actually be rendered on MDN yet, so it wasn't really a priority to review, but let's now attempt to get this data merged and worry about additional tests and MDN rendering later.

This is very well researched and matches what is in the nodejs docs. Great work!
I think there a bit of misinterpretation of how we use alternative_name, so I have a few things to change but overall I'm happy with this. Thank you!

It would be great if you could also rebase against latest master, so that we make sure all the data still passes the latest linting tests.

Thank you again! 👍

api/Console.json Outdated Show resolved Hide resolved
api/Console.json Outdated Show resolved Hide resolved
api/Console.json Outdated Show resolved Hide resolved
api/Console.json Outdated Show resolved Hide resolved
api/Performance.json Show resolved Hide resolved
@freaktechnik
Copy link
Contributor Author

Alright, rebased and the alternative_names usage reduced.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick updates @freaktechnik and sorry again this took enormously long! Happy to merge this now and really appreciating your patience and commitment to help this project. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:browsers 🌍 Data about browsers (versions, release dates, etc). This data is used for validation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants