Add `manifest-exists` rule and make other improvements #54

alrra · 2017-03-21T14:52:23Z

Add rule to check the existence of a web app manifest file (ref: Add rule(s) to check the usage of the Web App Manifest file #32).
Fix error in rule-runner when using element::name::index.
Fix manifest-exists rule .
Make rule-runner allow to specify request results.

Add rule to check if a single web app manifest file is specified, * and if that specified file is accessible.

The `::index` part wasn't being removed when calling the event and caused an exception.

alrra · 2017-03-21T14:58:05Z

src/lib/collectors/jsdom.ts

@@ -98,7 +100,8 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {

        return {
            body,
-            headers: response.headers
+            headers: response.headers,
+            statusCode: response.statusCode


@molant At some point rules will also need to know the request headers, so maybe we should rename headers to responseHeaders, or better yet just split this into 2 objects ({ request: {}, response: {} }). What do you think?

Can you give me an example of why a rule should know the request headers? If they need an specific header they can pass it via customHeaders.
If we need something else other than the headers from the response then I'd go for the 2 objects. If it is only the headers then responseHeaders and requestHeaders are ok.

Can you give me an example of why a rule should know the request headers?

While users can make extra requests where they can set if needed custom request headers so that they control exactly how the requests are made, I think that providing the request headers will simplify some things, and in certain cases could potentially avoid some extra requests (e.g.: users can check what headers where sent by default and based on that conditionally do extra things, they can quickly use request headers while debugging).

That being said, I don't expect them to be used much, probably mostly with rules related to: content negotiation.

If we need something else other than the headers from the response

Another question is: in the future, are we considering providing any other information about the requests (e.g.: information about timing, security)?

My opinion right now is that, even if we don't expose the request headers for now, I think the response related data should be under response: {...} so that we can add other things in the future without any problems.

OK, let's go with request: {}, response: {} then :)

alrra · 2017-03-21T15:16:13Z

src/tests/helpers/rule-runner.ts


-        t.context.rule[event.name](eventData);
+            event.responses.forEach((respons, i) => { // eslint-disable-line no-loop-func
+                ruleContext.fetchContent.onCall(i).returns(Promise.resolve(respons));


Note: When we change this to use fixtures, another thing to consider is that it should also allow to test the value that is passed to fetchContent.

molant · 2017-03-21T16:25:52Z

src/lib/collectors/jsdom.ts

@@ -98,7 +100,8 @@ const builder: CollectorBuilder = (server: Sonar, config): Collector => {

        return {
            body,
-            headers: response.headers
+            headers: response.headers,
+            statusCode: response.statusCode


Can you give me an example of why a rule should know the request headers? If they need an specific header they can pass it via customHeaders.
If we need something else other than the headers from the response then I'd go for the 2 objects. If it is only the headers then responseHeaders and requestHeaders are ok.

molant · 2017-03-21T16:28:52Z

src/lib/rules/manifest-exists/manifest-exists.md

+
+In the context of [progressive web
+apps](https://en.wikipedia.org/wiki/Progressive_web_app),
+providing a web app manifest files is essential.


I think we need more details here of what things are going to be checked. Cases where the rule will fail and when it will pass. You are checking if the file exists, that should be mentioned here:

Missing manifest

Duplicate manifest

File doesn't exist

Something else?

I think we need more details here of what things are going to be checked.

Note: We should discuss at one point what exactly should the docs contain.

This is the information that will show up in the website so it needs to be as complete as possible. Things it should have:

Why this rule is important and when to care about it

How to configure it if applicable

How it behaves and what does it test exactly:

Examples that pass the rule

Examples that trigger the rule

Special requirements if applicable (there might be rules that require an specific feature not supported in all collectors)

Related links (spec, articles, etc.)

Made an update before you made the above comment.

Will update / open anther PR with more information tomorrow.

molant · 2017-03-21T16:29:46Z

src/tests/helpers/rule-runner.ts


 import { test, ContextualTestContext } from 'ava'; // eslint-disable-line no-unused-vars
-import { Rule, RuleBuilder, ElementFoundEvent } from '../../lib/types'; // eslint-disable-line no-unused-vars
+import { Rule, RuleBuilder, ElementFoundEvent, FetchResponse} from '../../lib/types'; // eslint-disable-line no-unused-vars


If you have a better name for FetchResponse please let me know. I'm having a hard time finding good and meaningful names :(

Since in developer tools, that kinda of information is displayed in the network panel, maybeNetworkData?

Good to me, can you update the references?

molant · 2017-03-21T16:39:03Z

src/tests/helpers/rule-runner.ts


-        t.context.rule[event.name](eventData);
+            event.responses.forEach((respons, i) => { // eslint-disable-line no-loop-func


Can you rename this to response or res if it is already used?

Done, sorry about the typo.

* Change `rule-runner` to allow to specify the results for requests made by `fetchContent`. * Add `statusCode` member to the `FetchResponse` interface. * Update, improve, and fix the code for the `manifest-exists` rule.

Ref: #54 (comment)

Move members of `NetworkData` interface into more specialized interfaces (i.e. `Request` and `Response`), and add those more specialised interfaces as members of `NetworkData`. This change is done in order to remove the need to work around potential naming conflicts (i.e. both the `request` and `response` have `headers`), and thus, easily allow future additions (e.g.: adding information about network timing, network security, etc.), without requiring any changes. - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Ref: #54 (comment)

alrra and others added 3 commits March 21, 2017 16:49

Add rule to check the existence of a manifest file

9a29246

Add rule to check if a single web app manifest file is specified, * and if that specified file is accessible.

Fix error in rule-runner when using element::name::index

608c373

The `::index` part wasn't being removed when calling the event and caused an exception.

Fix manifest-exists rule

6df65f4

alrra assigned molant Mar 21, 2017

alrra requested a review from molant March 21, 2017 14:52

alrra commented Mar 21, 2017

View reviewed changes

molant requested changes Mar 21, 2017

View reviewed changes

alrra added 2 commits March 21, 2017 21:35

Make rule-runner allow to specify request results

99cf297

* Change `rule-runner` to allow to specify the results for requests made by `fetchContent`. * Add `statusCode` member to the `FetchResponse` interface. * Update, improve, and fix the code for the `manifest-exists` rule.

Use NetworkData instead of FetchResponse

8ee1076

Ref: #54 (comment)

molant approved these changes Mar 21, 2017

View reviewed changes

molant merged commit 162cc6b into webhintio:master Mar 21, 2017

alrra deleted the add-manifest-exists-rule-and-make-improvements branch March 21, 2017 20:05

alrra mentioned this pull request Apr 18, 2017

Add rule(s) to check the usage of the Web App Manifest file #32

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `manifest-exists` rule and make other improvements #54

Add `manifest-exists` rule and make other improvements #54

alrra commented Mar 21, 2017

alrra Mar 21, 2017 •

edited

molant Mar 21, 2017

alrra Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017 •

edited

molant Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017

molant Mar 21, 2017

alrra Mar 21, 2017


		t.context.rule[event.name](eventData);
		event.responses.forEach((respons, i) => { // eslint-disable-line no-loop-func

Add manifest-exists rule and make other improvements #54

Add manifest-exists rule and make other improvements #54

Conversation

alrra commented Mar 21, 2017

alrra Mar 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrra Mar 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `manifest-exists` rule and make other improvements #54

Add `manifest-exists` rule and make other improvements #54

alrra Mar 21, 2017 •

edited

alrra Mar 21, 2017 •

edited