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.

Sign up for GitHub

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

Jump to bottom

html/head/body tags should not be added automatically to an existing file/partial #822

Open
jarmo opened this issue Apr 16, 2022 · 3 comments
Open

html/head/body tags should not be added automatically to an existing file/partial #822

jarmo opened this issue Apr 16, 2022 · 3 comments
Labels
feature request New feature or request

Comments

@jarmo
Copy link

jarmo commented Apr 16, 2022

Describe the bug
I used pwa-asset-generator with --index flag to automatically add all splash image html tags for iOS. It added these tags as expected. However, it also added html, head and body tags which was unexpected:

<html>
  <head>
    <link rel="apple-touch-startup-image" ..>
     ...
  </head>
  <body>
  </body>
</html>

Since my "index" file is not an actual index, but is a separate partial html file, which is included by the html templating language I'm using into an actual "index.html" head tag then this broke my html and PWA (iOS actually doesn't work properly like this).

To Reproduce
Steps to reproduce the behavior:

  1. npx pwa-asset-generator pwa-icon.jpg -i partial-head.html
  2. Open partial-head.html
  3. See additional tags

Expected behavior

pwa-asset-generator should not automatically add/modify any html, head and body tags when it is modifying a file instead of creating a new one. Or there should be a way to disable this behavior with an extra flag.

Anyway, this behavior was unexpected and unnoticed at first and it broke PWA.

System (please complete the following information):

  • cli version 6.0.8
@jarmo jarmo added bug Something isn't working needs verification Bug needs to be verified with reproduction labels Apr 16, 2022
@onderceylan onderceylan removed the needs verification Bug needs to be verified with reproduction label Apr 18, 2022
@onderceylan
Copy link
Collaborator

Hi @jarmo, thanks for reporting this use case and sharing your feedback.

pwa-asset-generator was not designed to make edits to partial html files. I will mark it as a feature request and consider this improvement as part of the roadmap.

@onderceylan onderceylan added feature request New feature or request and removed bug Something isn't working labels Apr 18, 2022
@jarmo
Copy link
Author

jarmo commented Apr 19, 2022

@onderceylan thank you for your reply.

I would like to argue (in a friendly way, of course) that pwa-asset-generator's job is to create all the necessary images/icons/manifest.json and HTML tags, but it should not change the hierarchy of the html document like this, because this is totally unexpected.

When you think about it then index.html would be 99% of times a valid HTML already when it is an existing file and there's no need to add parent nodes nor create head tag. If it is not valid then it's definitely not pwa-asset-generator's job to fix that. If that file does not exist and pwa-asset-generator creates it then I can understand it trying to be a little more helpful by adding all these extra tags to create a valid html document.

I would either expect it to throw an error when it can't find head tag - then I as a developer would probably add temporarily these tags into partial to trick pwa-asset-generator into thinking that this is actually an index.html file and not some partial. In this way when I added these tags explicitly there's a less chance of creating an invalid html because I would remember to remove these tags. Or I would expect pwa-asset-generator to only add tags related to PWA-s and not add any other tags.

Since partials are not something esoteric in web application frameworks and current behavior of pwa-asset-generator breaks PWA completely when these are used then I would still consider this as a bug and not a feature.

All in all, thanks for this nice tool, which has helped me to spare some time developing PWAs.

@T2L
Copy link

T2L commented Nov 21, 2022

This feature would simplify everyone's life a lot. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jarmo @T2L @onderceylan