Closed Bug 1250104 Opened 4 years ago Closed 4 years ago
Import the jsesc library
13.70 KB, patch
|Details | Diff | Splinter Review|
Attachment #8721949 - Flags: feedback?(rnewman)
Here's the last thread I remember about using third-party libraries. https://mail.mozilla.org/pipermail/firefox-dev/2015-December/003697.html It didn't reach an actionable consensus. Dave, build folks: do you already have something in mind for where to put snapshots of third-party (or first-party but GitHub-canonical) libraries, particularly JS libraries?
Comment on attachment 8721949 [details] [diff] [review] patch.diff The code looks fine (modulo being 'webby' JS). We'd need to have some kind of documentation near this that indicates where it comes from, who owns updating it, what to do if there's a bug, etc. -- and probably some tests.
Attachment #8721949 - Flags: feedback?(rnewman) → feedback+
The way I'm importing this code currently is with a `cat fx-header jsesc.js > jsesc-fx.js`. I attached the `fx-header` to this bug. I just added there some information about how to update it, where it leaves etc. As for tests, there are tests running in there (https://github.com/mathiasbynens/jsesc/blob/master/tests/tests.js), but I'm not sure how I should integrate them with our tests. Should I take a subset of them and make them run with xpcshell?
There is no convention on where to put 3rd party JS projects. Various groups have imported code into their own directories, which I see is the approach in this patch. A risk with that is we'll have multiple copies or versions of the same package in the tree and in the distribution. I would recommend toolkit/modules/third_party or browser/modules/third_party as where to put these packages.
As this looks to be for services I would put something like services/third_party/jsesc/jsesc.js and also add a README alongside explaining what the current version is, where it came from and what the process is for updating it from the source. You may want to add services/third_party to the top-level .eslintignore file.
Here is the new patch with your comments addressed. I still need to add some testing, but as I'm unclear as to what should be tested here (it's already tested upstream), I haven't done anything here. Let me know if I can do anything to improve this patch so it can land :)
Attachment #8723008 - Flags: review?(rnewman) → review+
Attachment #8723512 - Attachment is obsolete: true
Attachment #8723538 - Attachment is obsolete: true
Attachment #8723008 - Attachment is obsolete: true
Attachment #8723543 - Attachment is obsolete: true
Updated with the .eslintignore.
The test this added is failing like https://treeherder.mozilla.org/logviewer.html#?job_id=7602831&repo=fx-team I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/bab8e722f1aa
I'll note that these failures did show up in the try push.
Thanks for the feedback, and sorry that you had to back this out. I don't have any windows machine to debug this. Anyone could take a look (or point me to someone who can)?
Flags: needinfo?(alexis+bugs) → needinfo?(wkocher)
You can probably do most of it just pushing to try and requesting windows xpcshell tests, but you could also request a loaner: https://wiki.mozilla.org/ReleaseEngineering/How_To/Request_a_slave
Attachment #8735945 - Attachment is obsolete: true
I believe the failing test in threeherder (https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d9d4ed8ca8&selectedJob=18749081) is not related to the changes I'm making here.
Attachment #8723622 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.