mozilla / addons-linter

🔍 Firefox Add-ons linter, written in JavaScript. 👁
Mozilla Public License 2.0
314 stars 142 forks source link

Turn MANIFEST_THEME_LWT_ALIAS into a linting error once support for these aliases is removed in Firefox #2463

Closed rpl closed 5 years ago

rpl commented 5 years ago

Describe the problem and steps to reproduce it:

As a follow up for #2259, once the support for these LWT theme aliases is going to be removed from Firefox, we should turn the MANIFEST_THEME_LWT_ALIAS (which is being added as part of #2283) from a linting warning to a linting error.

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1472740#c10, the linter should start to mark this as a linting error when Firefox 69 reached the beta channel.

wagnerand commented 5 years ago

To be honest, I am not sure I agree with the timeline in aforementioned bug comment. Doing so prevents themes and updates to be submitted for the current release version, which is where the majority of our user-base is. It will also exclude ESR for a longer period, which is another concern.

What do you think about keeping this a warning until at least until 71 hits release (assuming the Firefox patch makes it to 70 release), or even until ESR 68 is out of support?

rpl commented 5 years ago

@wagnerand keeping them as a warning sounds ok to me too.

In the patch that we are landing on Firefox 70 we are also changing the warning message to point out explicitly that Firefox >= 70 will ignore the deprecated aliases and so the linter warnings will also mention the new expected behavior once the Firefox 70 webext schema API will be imported in the linter.

In the meantime I've added the state: do not merge label on #2694, to be sure that we don't merge it.

wagnerand commented 5 years ago

Oh wait, we won't actually exclude submissions for those users since they are on versions that already support the new aliases, right? I assume the new aliases were added before 68?

If so, then we should actually merge this soonish, to avoid users accidentally submitting files with old aliases.

Sorry about that, I misinterpreted the initial impact of this change.

rpl commented 5 years ago

Oh wait, we won't actually exclude submissions for those users since they are on versions that already support the new aliases, right? I assume the new aliases were added before 68?

Looking at the compatibility table on MDN (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/theme#Colors) both LWT aliases and the related chromium-compatible property names are available way before 68.

If so, then we should actually merge this soonish, to avoid users accidentally submitting files with old aliases.

Yes, that was my thought and original intention with #2694, avoid that new submission may contain the ignored property name by mistake.

Looping in @nt1m to double-check that we are not missing any detail that he may be aware of.

AlexandraMoga commented 5 years ago

@wagnerand can this be verified already or do I need to wait for the next linter release?

wagnerand commented 5 years ago

You'll probably want to wait for a linter release. I think @EnTeQuAk will do one shortly.

AlexandraMoga commented 5 years ago

Verified fixed on stage with Fx68, Win10x64

Deprecated LWT props are raising a linting error now:

image