New: Add rule to check the usage of web app manifest's short_name & name
#498
Conversation
alrra
requested review from
austinkelleher,
molant,
patrick-steele-idem and
sarvaje
as code owners
There was a problem hiding this comment.
In the docs you said:
if it's over 12 characters also have a
short_namemember that is at most 12 characters.
but in the tests you have a few cases where name <= 12, short_name is declared and the test passes.
It is not clear in the examples that fail if it should fail or not. IMO it should because it is not needed.
|
|
||
| ### Examples that **pass** the rule | ||
|
|
||
| Manifest is specified with a `name` shorter than 30 characters |
There was a problem hiding this comment.
Does not having a manifest pass the rule?
If so we should probably tell so.
There was a problem hiding this comment.
If so we should probably tell so.
Done.
| // Requirements | ||
| // ------------------------------------------------------------------------------ | ||
|
|
||
| const { ucs2 } = require('punycode'); |
There was a problem hiding this comment.
punycode is deprecated
We should use Punycode.js
There was a problem hiding this comment.
I'm using Punycode.js, just that it should be a dependency not a devDependency.
There was a problem hiding this comment.
For whatever reason I though we weren't using it before, I didn't see an update in package.json and I assumed the worst :P
There was a problem hiding this comment.
For whatever reason I though we weren't using it before,
We used to use it in a rule, but now it's only used in making sure we can use the correct amount of emoji in commit messages.
just that it should be a dependency not a devDependency.
Fixed.
| const { ucs2 } = require('punycode'); | ||
|
|
||
| import { debug as d } from '../../utils/debug'; | ||
| import { IManifestFetchEnd, IResponse, IRule, IRuleBuilder } from '../../types'; // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
With the latest TS parsers you probably don't need the //eslint-disable-line no-unused-vars. I'm doing a PR that removes most of them.
| // * it's specified | ||
| // * if `name` is over the `short_name` length limit | ||
|
|
||
| if (shortName || shortNameIsRequired) { |
There was a problem hiding this comment.
I'm not sure this condition is right. Maybe something like this to make it clearer?
if (!shortName && !shortNameIsRequired) {
return;
}
// There is something wrong with `shortName` so we need to check what it is
await checkIfDefined(resource, shortName, 'short_name');
await checkIfNonEmpty(resource, shortName, 'short_name');
await checkIfUnderLimit(resource, shortName, 'short_name', shortNameLengthLimit); | }, | ||
| { | ||
| name: `Manifest is specified with 'name' and 'short_name'`, | ||
| serverConfig: { |
There was a problem hiding this comment.
Shouldn't this test fail? name.length is <= 12
I don't see any reports
There was a problem hiding this comment.
No. If name is up to 12 characters, then it can be used instead ofshort_name. However, if short_name is also specified, it's perfectly valid, and not a problem.
There was a problem hiding this comment.
This point of the documentation makes me think it should fail:
If the
nameis under or 12 characters, there isn't a need to specifyshort_nameas browsers can and will just usename.
If they will just use name if less than 12 characters, then short_name shouldn't be there. If they can decide what to use depending on the context and the space (list of apps, title below and icon) then I think this phrase should be reformulated.
There was a problem hiding this comment.
then I think this phrase should be reformulated.
Any suggestions?
| }, | ||
| { | ||
| name: `Manifest is specified with 'name' and empty 'short_name'`, | ||
| reports: [{ message: `Manifest should contain non-empty 'short_name' member` }], |
There was a problem hiding this comment.
Also shouldn't it fail because short_name isn't needed?
| serverConfig: { | ||
| '/': htmlWithManifestSpecified, | ||
| '/site.webmanifest': '' | ||
| } | ||
| }, | ||
| { | ||
| name: `Web app manifest is specified and request for file fails`, | ||
| reports: [{ message: `Web app manifest file request failed` }], | ||
| name: `Manifest is specified and request for file fails`, |
There was a problem hiding this comment.
we should also say in the test name that we expect to pass in this case (and for similar ones where fetching the manifest fails).
|
@molant This PR is ready for another review. |
| ``` | ||
|
|
||
| Note: [Not specifying a manifest file](manifest-exists.md), or having | ||
| an invalid one are covered by other rules, so those cases won't make i |
Fix #136