how to include third party javascript libraries?

[fippo]

In this chrome CL I tried adding a third-party library for dealing with SDP in webrtc tests. I put a trimmed down version of the code, a README specifying where to obtain it and a LICENSE file into a directory under the one with the tests using it. @foolip was summoned and asked me to raise the issue here.

How should javascript libraries that are third-party be treated? "Third party" in the sense "they existed before, they are independent from wpt and someone else maintains them"

I assume fetching them from the network is off the table since there should be no runtime dependency on the network.

Should they put in a central place so they can be ignored by linters? If they are only used by a particular spec (nothing else on the web uses SDP thankfully) is a place like <spec>/resources/ better. Should a package manager (npm, yarn, ...) be used? With or without lockfiles etc. What is the process for updating them (e.g. npm audit)? Should local modifications be avoided? Are there licensing concerns that need to be taken care of?

Lots of questions... in this particular case i'm happy to use the same pattern that is used for webidl2.js

[jgraham]

My point of view:

• Use <spec>/third_party for third party code
• We can write a rule to exclude all third_party directories from linting
• The best way to add them is typically using git subtree, in case local modifications are required (but local modifications are discouraged in general I'd say). Note that merging subtrees required a direct push to preserve the tree shape.
• I don't think we want to use a package manager here if possible. The case where it might not be is where we can't include source directly but need to include the output of a build step.
• Updating is hard; when we vendor stuff we lose the support of most tools that can check for outdated versions (I think).
• License needs to be compatible with the overall BSD license. So things like GPL dependencies are problematic (by which I think I mean "not possible to include")

[Hexcles]

The best way to add them is typically using git subtree, in case local modifications are required (but local modifications are discouraged in general I'd say). Note that merging subtrees required a direct push to preserve the tree shape. I don't think we want to use a package manager here if possible. The case where it might not be is where we can't include source directly but need to include the output of a build step. Updating is hard; when we vendor stuff we lose the support of most tools that can check for outdated versions (I think).

Vendoring third-party code and using a package manager are not mutually exclusive, if we choose not to use git subtree.

For npm packages, I actually think we should use a package manager, and check in the fetched source code. This solves the problem both for vendors (use the checked in source code directly) and authors (easy to upgrade using npm up), at the cost of losing the detailed version history (which doesn't really matter as long as we don't make local modifications). We probably want to use yarn for its flat mode.

[fippo]

I took a stab at the "use yarn" approach. Branch here

The package.json files can the pretty small thankfully, only listing dependencies. npm and yarn complain about a missing license field but that is just a warning. Checking in node_modules required git add -f since node_modules is in the root .gitignore. -f solves the problem however.

One thing I didn't like was checking in the full package including documentation, tests etc. I think having a .gitignore to just :cherries:-pick what is needed may reduce the size considerably. But when i looked at least in the case of the sdp package the tests were already not included in the npm package.

[fippo]

made a PR from the branch

[snuggs]

@fippo this can turn into a ton of technical debt very fast as i'm sure you already know. Also not sure what happens from breaking typical node conventions of traversing node_modules location. Then I realized this isn't a node project per se and the minimum we need is sdp.js. Luckily this package is fairly tight. So seems like an easy band aid. Our future selves may not be so lucky with other third_party so great to discuss. :-).

My vote/opinion is for placing sdp.js (wherever is agreed upon), reference it where needed, and call it a day. However this strategy still doesn't avoid needing to check externally for updates. It's already commonplace (i.e. Tree Shaking) to strip what we don't need (down to the method). If we need a license can throw it at the head. Perhaps i'm thinking too lazy but never been told that's a bad thing 😎

FWIW just my 2 satoshis. 👍 👍 on the sdp js lib though. Personally it's been helpful numerous times in the past. 🙏

[fippo]

ping - if you folks could take a look at the PR

@alvestrand just proposed another creative test in which this library would be super helpful :-)

[stephenmcgruer]

Hi all, sorry for the delay. I discussed this with @jgraham and @Hexcles today, and we're happy for the proposal from @snuggs to go ahead. That is:

i. Add a new directory, webrtc/third_party/sdp ii. Vendor (copy) sdp.js and its associated LICENSE file into webrtc/third_party/sdp iii. Add lint rules to ensure that third_party directories are excluded from linting presubmits.

This is us (deliberately) deferring on an overall policy for third-party javascript in WPT, but hopefully unblocking your current efforts.

I am happy to work on (iii), and look to yourselves to arrange (i) and (ii), if this meets your needs. Thanks!

[fippo]

Thank you (also Harald for pinging)! Made a new PR referenced above this comment, lint seems to be a solved problem :heart:

[foolip]

As part of https://github.com/web-platform-tests/wpt/pull/21855 I've spotted another third party JavaScript library in the repo, https://github.com/nodeca/pako added in https://github.com/web-platform-tests/wpt/pull/19614.

[zcorpan]

Noting for the record another PR adding third-party code: https://github.com/web-platform-tests/wpt/pull/23789

It seems to me that right now https://github.com/web-platform-tests/rfcs/issues/46#issuecomment-605098405 is the de-facto overall policy.

[jgraham]

I am somewhat concerned by PRs like that randomly adding third party code under different licenses all over the tree. @foolip started looking at lints for this; I wonder if we could check whether people are adding either LICENSE files or code with a license block outside of third_party directories, and make that check impossible to disable, in addition to the proposed check for LICENSE files in general with a list of vetted directories.

[foolip]

I've commented on https://github.com/web-platform-tests/wpt/pull/21855#issuecomment-641217990 about the difficulties I ran into and closed the PR to make it clear I'm not making any progress on it, and don't expect to.

[stephenmcgruer]

So, we can never stop people adding third party code without us knowing; they could simply copy stuff from any library into a test file and there's no way we can detect that.

So, we should concentrate on common things people may do that we can detect, which to me is this list:

• Introduce a third_party/foo directory
• Add a LICENSE or LICENSE.md file (basically any file with LICENSE in the name)
• Add a file with a license block in it (I'm not clear how detectable this one is).

It seems to me that adding third-party libraries should be relatively rare, so perhaps we should be strict about this. What about having the following lints:

1. A path-lint checking for modifying a file with third_party/ in its name.
2. A path-lint checking for modifying a file named LICENSE (or with that text in its name).

Then we would explicitly allow specific third_party directories in lint.ignore, which today would be:

*: webrtc/third_party/sdp/*
*: compression/third_party/pako/*

[jgraham]

I think that makes sense, although I'm not fully clear on the details of what you want to lint. I think we can enforce two things:

• Every file with LICENSE in its name is a lint error unless it's in a directory called third_party
• Every third_party directory is an error unless it's listed in the ignore rules. In those rules we should break down the listing by license to set an expectation that adding specific licenses is OK and unknown ones require more investigation.

Other files with a license block are somewhat detectable with a regex lint although it's obviously going to be pretty best-effort.

[stephenmcgruer]

• Every file with LICENSE in its name is a lint error unless it's in a directory called third_party
• Every third_party directory is an error unless it's listed in the ignore rules. In those rules we should break down the listing by license to set an expectation that adding specific licenses is OK and unknown ones require more investigation.

Yep, these are what I want to lint :). (actually I had missed the first one, which I think is great!).

Ok, I'll work on the lint change, and maybe do some documentation work too.

[stephenmcgruer]

Every file with LICENSE in its name is a lint error unless it's in a directory called third_party

Violations of this rule:

ERROR:lint:webgpu/LICENSE.txt: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:css/CSS2/LICENSE-BSD: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:css/CSS2/LICENSE-W3CD: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:css/CSS2/LICENSE-W3CTS: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:css/css-color/LICENSE: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:fonts/CSSTest/LICENSE: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:fonts/adobe-fonts/LICENSE: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:fonts/noto/NotoSansAdlam-hinted/LICENSE_OFL.txt: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:fonts/noto/NotoSansCypriot-hinted/LICENSE_OFL.txt: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:html/canvas/tools/LICENSE.txt: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)
ERROR:lint:css/css-ui/support/PTS/PngSuite.LICENSE: License files (e.g. LICENSE.md) must be grandchildren of a third_party directory, e.g. third_party/foo/LICENSE.md (LICENSE-LOCATION)

[jgraham]

@foolip looked at some of these

https://bugzilla.mozilla.org/show_bug.cgi?id=1637924 is filed for the css-color issue but is waiting for @dbaron I think we can also assume that fonts is all third party. So that leaves webgpu, CSS2, html/canvas/tools and css/css-ui/support/PTS/ to investigate.

[stephenmcgruer]

CSS2 is https://github.com/web-platform-tests/wpt/pull/23593, which is waiting on @wseltzer I believe.