feat: detect API and manifest key incompatibilities with strict_min_version (#1493) #2290

freaktechnik · 2018-11-23T21:30:48Z

This is a first attempt at integrating the browser-compat-data from the MDN compatibility tables with the linter.
It currently checks for manifest keys, permissions and APIs to be supported by the given strict_min_version in the manifest for both Firefox and Firefox for Android. All produced messages are warnings. However, no warning is shown if Firefox or Firefox for Android do not implement an API and that is documented in the compat data. This is to avoid a lot of Firefox for Android spam, plus the totally unsupported APIs should be caught by the schemas.

I'm not quite happy with the amount of duplication of logic, even though it's subtly different for all cases, so improvement suggestions welcome! Further I'll happily add more tests, these are just the ones I could come up with.

freaktechnik · 2018-11-23T21:31:05Z

README.md

@@ -151,7 +151,7 @@ Dependencies are automatically kept up-to-date using [greenkeeper](http://greenk
 | npm run test-coverage-once      | Runs the tests once with coverage                                                |
 | npm run test-integration-linter | Runs our integration test-suite                                                  |
 | npm run prettier                | Automatically format the whole code-base with Prettier                           |
-| npm run prettier-dev            | Automatically compare and format modified source files against the master branch   |
+| npm run prettier-dev            | Automatically compare and format modified source files against the master branch |


Uh, this must be a side-effect from running prettier...

Indeed it may, it's interesting that this hasn't been caught before though 🤷‍♂️

freaktechnik · 2018-11-24T13:15:34Z

However, no warning is shown if Firefox or Firefox for Android do not implement an API and that is documented in the compat data. This is to avoid a lot of Firefox for Android spam, plus the totally unsupported APIs should be caught by the schemas.

To expand on this a litte: if the linter should warn about these APIs that are marked as not supported but are in the schema, those should be separate rules/messages, since they will always apply, not matter the strict_min_version.

It may also make sense to group these warnings by being able to tell the linters what applications should even be considered, this patch adds three messages for each Firefox and Firefox for Android, having general messages would add another three messages per application for general missing support of an api, manifest key or permission.

Arguably those could re-use the schema warnings, however those are application independent and there's quite a big gap between Firefox and Firefox for Android in terms of API support.

EnTeQuAk

Unless I'm mistaken, merging webextension-api-compat.js and webextension-api-compat-android.js should be perfectly possible since the only real difference is reporting and the isCompatible(..., 'firefox') vs isCompatible(..., 'firefox_android') call. Am I missing someting? If so, I think I'd prefer us doing so.

Anyhow, this is looking like a great start for a very bright future! Thanks a lot for tackling this. I'd like to test this more extensively probably today / tomorrow so I only left a first batch of comments.

cc @rpl on this for second pair of eyes to get this feature right.

EnTeQuAk · 2018-11-27T11:10:43Z

README.md

@@ -151,7 +151,7 @@ Dependencies are automatically kept up-to-date using [greenkeeper](http://greenk
 | npm run test-coverage-once      | Runs the tests once with coverage                                                |
 | npm run test-integration-linter | Runs our integration test-suite                                                  |
 | npm run prettier                | Automatically format the whole code-base with Prettier                           |
-| npm run prettier-dev            | Automatically compare and format modified source files against the master branch   |
+| npm run prettier-dev            | Automatically compare and format modified source files against the master branch |


Indeed it may, it's interesting that this hasn't been caught before though 🤷‍♂️

EnTeQuAk · 2018-11-27T11:11:44Z

src/messages/manifestjson.js

+}
+
+export const PERMISSION_FIREFOX_UNSUPPORTED_BY_MIN_VERSION =
+  'èERMISSION_FIREFOX_UNSUPPORTED_BY_MIN_VERSION';


I think this should be PERMISSION instead of èERMISSION` I think

src/utils.js

freaktechnik · 2018-11-27T11:35:58Z

Unless I'm mistaken, merging webextension-api-compat.js and webextension-api-compat-android.js should be perfectly possible since the only real difference is reporting and the isCompatible(..., 'firefox') vs isCompatible(..., 'firefox_android') call. Am I missing someting? If so, I think I'd prefer us doing so.

The reason I had split them is so they are different reported messages. Since some people may not care about android incompatibility.

EnTeQuAk · 2018-11-27T12:22:13Z

The reason I had split them is so they are different reported messages. Since some people may not care about android incompatibility.

They can continue to be different messages, just being raised by the same rule instead?

freaktechnik · 2018-11-27T12:25:47Z

From what I understand the eslint rules are mapped to codes here: https://github.com/mozilla/addons-linter/pull/2290/files#diff-8253f5d405f99dbf63dd1804d117413fR191

If there's a way to define the code with context.report I'd of course do that.

EnTeQuAk · 2018-11-27T12:37:08Z

From what I understand the eslint rules are mapped to codes here: /pull/2290/files#diff-8253f5d405f99dbf63dd1804d117413fR191

If there's a way to define the code with context.report I'd of course do that.

Eh, indeed, sorry I kinda had the wrong thing remembered… Can we maybe make most of the code in the rule a utility function and re-use it for both at least?

freaktechnik · 2018-11-27T23:08:54Z

Eh, indeed, sorry I kinda had the wrong thing remembered… Can we maybe make most of the code in the rule a utility function and re-use it for both at least?

Where should I move it? I'd optimally also move the common module imports of the browser compat data and the schema utils. And I wouldn't want to put that into utils.js, I don't think.

EnTeQuAk · 2018-11-28T06:30:47Z

Where should I move it? I'd optimally also move the common module imports of the browser compat data and the schema utils. And I wouldn't want to put that into utils.js, I don't think.

Right now most or all utilities that are used in rules live in utils.js so it's not the worst place to start. At some point, we may want a specific rule_helpers.js (or src/rules/utils.js which might be better) but I think utils.js might be fine. cc @rpl for a second opinion

ExE-Boss · 2018-11-28T12:31:00Z

I’d personally go with src/rules/utils.js.

EnTeQuAk · 2018-12-05T07:36:42Z

@freaktechnik do you have the time to update the pull request and maybe resolve conflicts in package-lock? Although there may be a few more updates coming to addons-liner in the next few days 🙈

freaktechnik · 2018-12-05T09:29:23Z

I do have the time though I wasn't expecting much activity this week...

…ersion (mozilla#1493)

freaktechnik · 2018-12-05T20:29:37Z

Sorry for the force pushes, I rebased onto master and hopefully made the package-lock a lot less likely to not be auto mergable. (I still don't understand why NPM sometimes does the lockfile with ^ and sometimes not, I know there's a version based difference but there also seem to be env based differences).

EnTeQuAk · 2018-12-11T07:36:55Z

(I still don't understand why NPM sometimes does the lockfile with ^ and sometimes not, I know there's a version based difference but there also seem to be env based differences).

This also has to do with the npm version (node 8 vs node 10 have a very clear difference here, I keep forgetting which version does what though)

EnTeQuAk

Looks really good now, thanks a ton for all this hard work! 💖

Let's get this going and see how it does in production.

* Pin dependencies to fixed version again * Don't ignore files and folders with a leading dot. (#2316) * Detect API and manifest key incompatibilities with strict_min_version (#1493) (#2290) * Move internal scripts to separate folder (#2309) * Various dependency updates

freaktechnik commented Nov 23, 2018

View reviewed changes

freaktechnik force-pushed the version-check branch from 1003a11 to 0a3d25c Compare November 23, 2018 21:34

EnTeQuAk self-requested a review November 27, 2018 11:09

EnTeQuAk reviewed Nov 27, 2018

View reviewed changes

freaktechnik force-pushed the version-check branch 2 times, most recently from 13e604a to 9dc16e7 Compare December 5, 2018 20:27

freaktechnik added 4 commits December 5, 2018 21:27

feat: detect API and manifest key incompatibilities with strict_min_v…

f531293

…ersion (mozilla#1493)

Fix typo in message code

1112bd0

Only add mdn-browser-compat-data to package-lock

65863c4

Move common rules code to utils

576704e

freaktechnik force-pushed the version-check branch from 9dc16e7 to 576704e Compare December 5, 2018 20:28

EnTeQuAk approved these changes Dec 11, 2018

View reviewed changes

EnTeQuAk merged commit fc1ddbc into mozilla:master Dec 11, 2018

freaktechnik mentioned this pull request Dec 11, 2018

Report unsupported APIs based on MDN compat data #2317

Closed

wbamberg mentioned this pull request Dec 12, 2018

Add addons-linter as a user of BCD mdn/browser-compat-data#3203

Merged

This was referenced Feb 21, 2019

Build(deps-dev): update web-ext requirement from ^2.2.2 to ^3.0.0 ianwremmel/browser-ruler#97

Closed

Bump web-ext from 2.9.3 to 3.0.0 sneakypete81/updatescanner#331

Merged

This was referenced Feb 22, 2019

Bump web-ext from 2.9.3 to 3.0.0 hypothesis/browser-extension#117

Merged

Bump web-ext from 2.9.3 to 3.0.0 armateam/extension#208

Merged

Update web-ext requirement from ^2.9.3 to ^3.0.0 amazon-alternatives/extension#33

Closed

This was referenced Mar 1, 2019

Bump web-ext from 2.9.1 to 3.0.0 dguo/color-tailor#59

Closed

Bump web-ext from 2.9.1 to 3.0.0 dguo/picture-paint#42

Closed

Bump web-ext from 2.9.1 to 3.0.0 dguo/chromatastic#39

Closed

dependabot-preview bot mentioned this pull request Jun 11, 2019

Bump web-ext from 2.9.1 to 3.0.0 OctoLinker/OctoLinker#581

Closed

This was referenced Jul 1, 2019

Bump web-ext from 2.9.1 to 3.1.0 dguo/picture-paint#60

Closed

Bump web-ext from 2.9.1 to 3.1.0 dguo/color-tailor#73

Closed

Bump web-ext from 2.9.1 to 3.1.0 dguo/chromatastic#52

Closed

This was referenced Jul 19, 2019

build(deps-dev): bump web-ext from 2.9.3 to 3.1.0 in /addon mozilla/code-coverage#52

Closed

build(deps-dev): bump web-ext from 2.9.3 to 3.1.1 in /addon mozilla/code-coverage#57

Merged

Bump web-ext from 2.8.0 to 3.1.1 remusao/site-pinning#2

Merged

This was referenced Aug 1, 2019

Bump web-ext from 2.9.1 to 3.1.1 dguo/chromatastic#59

Closed

Bump web-ext from 2.9.1 to 3.1.1 dguo/picture-paint#68

Closed

Bump web-ext from 2.9.1 to 3.1.1 dguo/color-tailor#83

Closed

dependabot-preview bot mentioned this pull request Aug 21, 2019

Bump web-ext from 2.6.0 to 3.1.1 jpelaezClub/mailvelope#21

Closed

dependabot-preview bot mentioned this pull request Aug 29, 2019

Bump web-ext from 2.9.3 to 3.1.1 jamestomasino/stutter#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: detect API and manifest key incompatibilities with strict_min_version (#1493) #2290

feat: detect API and manifest key incompatibilities with strict_min_version (#1493) #2290

freaktechnik commented Nov 23, 2018

freaktechnik Nov 23, 2018

EnTeQuAk Nov 27, 2018

freaktechnik commented Nov 24, 2018

EnTeQuAk left a comment

EnTeQuAk Nov 27, 2018

EnTeQuAk Nov 27, 2018

freaktechnik commented Nov 27, 2018

EnTeQuAk commented Nov 27, 2018

freaktechnik commented Nov 27, 2018 •

edited

Loading

EnTeQuAk commented Nov 27, 2018

freaktechnik commented Nov 27, 2018

EnTeQuAk commented Nov 28, 2018 •

edited

Loading

ExE-Boss commented Nov 28, 2018

EnTeQuAk commented Dec 5, 2018

freaktechnik commented Dec 5, 2018

freaktechnik commented Dec 5, 2018 •

edited

Loading

EnTeQuAk commented Dec 11, 2018

EnTeQuAk left a comment

feat: detect API and manifest key incompatibilities with strict_min_version (#1493) #2290

feat: detect API and manifest key incompatibilities with strict_min_version (#1493) #2290

Conversation

freaktechnik commented Nov 23, 2018

freaktechnik Nov 23, 2018

Choose a reason for hiding this comment

EnTeQuAk Nov 27, 2018

Choose a reason for hiding this comment

freaktechnik commented Nov 24, 2018

EnTeQuAk left a comment

Choose a reason for hiding this comment

EnTeQuAk Nov 27, 2018

Choose a reason for hiding this comment

EnTeQuAk Nov 27, 2018

Choose a reason for hiding this comment

freaktechnik commented Nov 27, 2018

EnTeQuAk commented Nov 27, 2018

freaktechnik commented Nov 27, 2018 • edited Loading

EnTeQuAk commented Nov 27, 2018

freaktechnik commented Nov 27, 2018

EnTeQuAk commented Nov 28, 2018 • edited Loading

ExE-Boss commented Nov 28, 2018

EnTeQuAk commented Dec 5, 2018

freaktechnik commented Dec 5, 2018

freaktechnik commented Dec 5, 2018 • edited Loading

EnTeQuAk commented Dec 11, 2018

EnTeQuAk left a comment

Choose a reason for hiding this comment

freaktechnik commented Nov 27, 2018 •

edited

Loading

EnTeQuAk commented Nov 28, 2018 •

edited

Loading

freaktechnik commented Dec 5, 2018 •

edited

Loading