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

feat(manifest): add useWebmanifestExtension option and improve docs #241

Merged
merged 10 commits into from Mar 12, 2020

Conversation

gangsthub
Copy link
Contributor

@gangsthub gangsthub commented Oct 15, 2019

According to the spec [W3] (and W3C):

The official file extension for the manifest is .webmanifest.

  • I added the option in the Manifest module but left the "json" as the default one for backward compatibility.

    Using .webmanifest is also recommended by Webhint.

  • I also added some tests (mocking the hasha module) updated the snapshots.

  • And edited the documentation to show the default options.

@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #241 into dev will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #241      +/-   ##
==========================================
+ Coverage   72.55%   72.64%   +0.08%     
==========================================
  Files          11       11              
  Lines         317      318       +1     
  Branches       98       98              
==========================================
+ Hits          230      231       +1     
  Misses         75       75              
  Partials       12       12              
Impacted Files Coverage Δ
lib/manifest/module.js 90.00% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 778735f...542d342. Read the comment docs.

@ricardogobbosouza
Copy link
Member

ricardogobbosouza commented Oct 15, 2019

@gangsthub it would be interesting to warn the user if the extension type is different from .webmanifest

@gangsthub
Copy link
Contributor Author

gangsthub commented Oct 16, 2019

I reverted mocking the package hasha as it was returning the mocked value in built files:

"fixture/.nuxt/dist/client/manifest.HASHMOCK.webmanifest",

But not in generated files (13d3892):

"fixture/dist/_nuxt/manifest.bee706d5.webmanifest",

And I didn't know why or how to fix it 🤔

@pi0
Copy link
Member

pi0 commented Oct 21, 2019

Hi @gangsthub and thanks for this PR ❤️ I'm so sad to say I think it's not a good idea to recommend using non-JSON extension by users. I'm still up with adding a simple flag (Like workbox.useWebmanifest: true) option to change extension to spec recommended one. Please reopen a new PR if you wish to. And still open to discussions here, a new issue or discord.

PS: I've tried to explain it as much as i can and add relative issues in a wiki page: https://github.com/nuxt-community/pwa-module/wiki/.webmanifest

@ricardogobbosouza
Copy link
Member

ricardogobbosouza commented Oct 21, 2019

@pi0 or should this PR change?
Prompt user if manifest file extension is different from json.

@pi0
Copy link
Member

pi0 commented Oct 21, 2019

@ricardogobbosouza I think so. Just adding a flag to change ext should be enough. There are enough reasons to not warn users, etc. And no reason to make it more complicated to allow any extension.

@gangsthub
Copy link
Contributor Author

gangsthub commented Oct 22, 2019

Noted. It's correct, it makes no sense to give freedom on choosing an extension different than json or webmanifest. I'll be:

  • changing the option,
  • deleting the warning and
  • updating the rest.

paul_melero added 3 commits Oct 22, 2019
@gangsthub gangsthub changed the title feat: add file extension option in manifest. Standard is .webmanifest feat: add file extension option in manifest to allow webmanifest Oct 22, 2019
@ricardogobbosouza ricardogobbosouza self-requested a review Dec 3, 2019
@ricardogobbosouza ricardogobbosouza requested a review from pi0 Dec 3, 2019
@Heziode
Copy link

Heziode commented Feb 19, 2020

Any update ?

@pi0 pi0 changed the title feat: add file extension option in manifest to allow webmanifest feat(manifest): add useWebmanifestExtension option and improve docs Mar 12, 2020
@pi0 pi0 merged commit 4484e6c into nuxt-community:dev Mar 12, 2020
3 checks passed
@pi0
Copy link
Member

pi0 commented Mar 12, 2020

Thanks, @gangsthub. I've merged and also updated the wiki to include webhint

@gangsthub gangsthub deleted the feat/fileExtension branch Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants