Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(installable-manifest): expose app manifest url #11330

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

adrianaixba
Copy link
Collaborator

for #9683

exposes the app manifest url in the installable manifest audit

@adrianaixba adrianaixba requested a review from a team as a code owner August 26, 2020 22:44
@adrianaixba adrianaixba requested review from connorjclark and removed request for a team August 26, 2020 22:44
*/
static async audit_(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
const manifestFailures = InstallableManifest.assessManifest(manifestValues);

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connor will know better but i think we want null rather than undefined

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea null is preferable for "missing", especially when the object may be JSON serialized (properties set to undefined are dropped in JSON)

*/
static async getWebAppManifest(passContext) {
const response = await passContext.driver.getAppManifest();
if (!response) return null;
return manifestParser(response.data, response.url, passContext.url);
const parsedManifest = manifestParser(response.data, response.url, passContext.url);
const manifestUrl = response.url;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we talked in voice about this.. should manifestUrl be part of the parser's return value? I said no, but... let's reconsider. ... it could. and it kinda makes sense. just spoke with @brendankenny and we both feel fairly ambivalent about it.

It keeps things a tad cleaner to put it in the manifestParser return, so let's do that. (well, as it turns out... you already did)

that actually means you can revert allllll the changes in gather-runner

anddddddddddd i think everything will just still end up working fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea it seems like this whole file could be reverted and the url from the parser lib will be consumable in installable-manifest

*/
static async audit_(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
const manifestFailures = InstallableManifest.assessManifest(manifestValues);

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined;

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

side note how do we have an audit that doesn't have a score.. so confused

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extends MultiCheckAudit and that audit_ life, def confusing though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OHHHH MultiCheckAudit. of course.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to Paul's comment, it seems you two worked out that just changing the return value of the manifest parser is enough to get installable-manifest the url. No need for any of the gather-runner changes afaict

An end-to-end to would be good here, can you add to the expectations file for one of our smoke tests? pwa1 and pwa2 both load pages that have a manifest, so you could add an assertion there for installable-manifest.

@@ -505,6 +505,7 @@ function parse(string, manifestUrl, documentUrl) {
raw: string,
value: manifest,
warning: manifestUrlWarning,
manifestUrl: manifestUrl,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think url would be a better name here, considering the natural way to name a variable holding this object is manifest (gather-runner actually uses parsedManifest which is also cool), and manifest.manifestUrl is redundant.

I think manifest.url would not lead to any confusion about what the url is referring to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should do this for the audit items value too? details.items[0].url vs details.items[0].manifestUrl. manifestUrl does add some clarity, although not sure if its necessary given that the audit refers to the manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url would be great there too

Copy link
Member

@brendankenny brendankenny Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some pretty good self documenting value for keeping it manifestUrl when it's deep in installableManifest.details.items[0], though, since we don't publish docs for specific audit's details.

Note also it's a term from the spec, to differentiate from "document URL" and start_url (which are both also needed for parsing the manifest).

@@ -584,7 +584,9 @@ class GatherRunner {
}

// Fetch the manifest, if it exists.
baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);
const manifestResult = await GatherRunner.getWebAppManifest(passContext);
// manifestResult could be null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we follow this comment style guide:

// Start with capital letter. End with punctuation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(oh, but it does seem like we can revert these few lines?)

*/
static async getWebAppManifest(passContext) {
const response = await passContext.driver.getAppManifest();
if (!response) return null;
return manifestParser(response.data, response.url, passContext.url);
const parsedManifest = manifestParser(response.data, response.url, passContext.url);
const manifestUrl = response.url;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea it seems like this whole file could be reverted and the url from the parser lib will be consumable in installable-manifest

*/
static async audit_(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
const manifestFailures = InstallableManifest.assessManifest(manifestValues);

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea null is preferable for "missing", especially when the object may be JSON serialized (properties set to undefined are dropped in JSON)

expect(result).toHaveProperty('raw', JSON.stringify(manifest));
expect(result && result.value).toMatchObject({
const parsedManifest = result ? result.parsedManifest : null;
expect(parsedManifest).toHaveProperty('raw', JSON.stringify(manifest));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be reverted to after you revert the changes in gather-runner, but I think an assertion here for result.url would be good.

@connorjclark connorjclark changed the title core: expose app manifest url on installable manifest audit core(installable-manifest): expose app manifest url Aug 28, 2020
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice job!

@connorjclark
Copy link
Collaborator

fyi if you put "fixed by #blah" in the first post, when the PR merges it auto-closes the associated issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants