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

gatsby-plugin-manifest doesn't allow app shortcuts #27927

Closed
ediblecode opened this issue Nov 9, 2020 · 11 comments · Fixed by #27951
Closed

gatsby-plugin-manifest doesn't allow app shortcuts #27927

ediblecode opened this issue Nov 9, 2020 · 11 comments · Fixed by #27951
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: bug An issue or pull request relating to a bug in Gatsby

Comments

@ediblecode
Copy link
Contributor

Description

Adding app shortcuts to the plugin options in gatsby-plugin-manifest 2.5.0+ results in an Invalid plugin options error

I think this was caused by enabling 'plugin option validation', released in #27437 and published in 2.5.0 of gatsby-plugin-manifest. The new options validation doesn't include the shortcuts option. It worked before 2.5.0 - I've just upgraded from 2.4.21 where it was working.

Steps to reproduce

  1. Install gatsby-plugin-manifest 2.5.0
  2. Add shortcuts to the plugin's options:
// gatsby-config.s
module.exports = {
  plugins: [
    {
      resolve: `gatsby-plugin-manifest`,
      options: {
+        shortcuts: [
+          { name: "Topics A to Z", url: "/topics/?utm_source=a2hs&utm_medium=shortcuts", }
+        ]
      }
    }
  ]
}

Expected result

Shortcuts are added to the generated manifest.webmanifest

Actual result

You get the following CLI error:

error Invalid plugin options for "gatsby-plugin-manifest": "shortcuts" is not allowed

Environment

System:
OS: Windows 10 10.0.19041
CPU: (4) x64 Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz
Binaries:
Node: 12.16.3 - C:\Program Files\nodejs\node.EXE
Yarn: 1.16.0 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 6.14.4 - C:\Program Files\nodejs\npm.CMD
Languages:
Python: 2.7.11
Browsers:
Chrome: 86.0.4240.183
Edge: Spartan (44.19041.423.0)
npmPackages:
gatsby: ^2.25.3 => 2.25.3
gatsby-plugin-catch-links: ^2.3.15 => 2.3.15
gatsby-plugin-eslint: ^2.0.8 => 2.0.8
gatsby-plugin-google-tagmanager: ^2.4.0 => 2.4.0
gatsby-plugin-manifest: ^2.5.2 => 2.5.2
gatsby-plugin-offline: ^3.3.2 => 3.3.2
gatsby-plugin-preact: ^4.0.16 => 4.0.16
gatsby-plugin-prefetch-google-fonts: ^1.4.3 => 1.4.3
gatsby-plugin-react-helmet: ^3.3.14 => 3.3.14
gatsby-plugin-sass: ^2.4.2 => 2.4.2
gatsby-plugin-sitemap: ^2.5.1 => 2.5.1
gatsby-plugin-split-css: ^1.0.2 => 1.0.2
gatsby-plugin-typescript: ^2.5.0 => 2.5.0
gatsby-react-router-scroll: 3.0.0 => 3.0.0

@ediblecode ediblecode added the type: bug An issue or pull request relating to a bug in Gatsby label Nov 9, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 9, 2020
@ediblecode
Copy link
Contributor Author

Looks like there are other properties missing too, see #27839

@barbalex
Copy link

barbalex commented Nov 9, 2020

Seems to me that was a breaking change - should have been expressed in the version number

@LekoArts LekoArts added topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 10, 2020
@ediblecode
Copy link
Contributor Author

@barbalex I think you're right! I see you've worked round it in #27839 by downgrading - I think I'll do the same for the timebeing.

I'm happy to submit a PR to add shortcuts and scope as properties - hopefully that'll fix both issues. I'll also do a quick scan of https://web.dev/add-manifest/#manifest-properties whilst I'm there to see if there's anything else obviously missing. Watch this space!

@mxstbr
Copy link
Contributor

mxstbr commented Nov 10, 2020

Not a breaking change, simply an oversight bug! Sorry about the troubles, looking forward to your PR @ediblecode 🙏 The code for this is in gatsby-plugin-manifest's gatsby-node.js.

@barbalex
Copy link

barbalex commented Nov 10, 2020

@mxstbr I wish I could build such a big code base with so few (noticeable) bugs. Congrats and thanks for an overall astoundingly great job!

@moonmeister
Copy link
Contributor

The validation is incomplete for manifest. I have an old PR with the correct validation. I'll try and get that pulled forward and merged. I'll post a PR link here when I have it.

@ediblecode
Copy link
Contributor Author

Perfect, thanks @moonmeister, I'll hold off until you've found that that then, thanks.

@ediblecode
Copy link
Contributor Author

And related, there's a new flag coming to disable option validation, see #27885 which will be a nicer workaround than downgrading

@moonmeister
Copy link
Contributor

#27951 has the changes from the old PR. The spec version that validator was built against is over a year old. If anyone want's to help get it updated that'd be great. Current spec is here: https://www.w3.org/TR/2020/WD-appmanifest-20201019/

@ediblecode
Copy link
Contributor Author

@moonmeister this looks great. I've been through with a fairly fine-toothed comb, and it looks like you've captured everything in the latest spec, so great work! I had a flick through various previous iterations of the spec, and although it's changed a lot, most are wording changes - it doesn't look like any properties have changed recently as far as I could tell.

There are only a few, minor things I've spotted but they really are nitpicks:

  • The categories member recommends "Manifest authors are encouraged to use lower-case." in the spec so mabye we could add .lowercase()? It doesn't really matter, because they'll get lowercased anyway.
  • The type member on ManifestImageResource is missing optional. (I know optional is optional but it's explicit everywhere else)
  • There's a supplementary spec (I know, not confusing at all) that contains extra members. The two relevant ones are platform and label. I suggest maybe we at least add label to screenshots because "For accessibility, authors are encouraged to provide a label for each screenshot."

@gillkyle
Copy link
Contributor

gillkyle commented Nov 12, 2020

And related, there's a new flag coming to disable option validation, see #27885 which will be a nicer workaround than downgrading

I actually paused that PR to disable validation for a couple reasons (#27885 (comment)) mainly because I think we just merged in a more ideal solution, which is warning about invalid options rather than failing to build. If we find that we really need the bypass flag we could add it, but I think cases like this would be covered by what was done in #27938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins-PWA Issues related to PWA: the gatsby-plugin-offline and gatsby-plugin-manifest plugins type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants