- Issue created by @Grevil
- Assigned to Grevil
- 🇩🇪Germany Grevil
I don't know what we are doing here, and I am really annoyed, that there are no comments for such a complex regex pattern...
So, the following questions arise:
Why are simply not all "style" elements stored? Why only the ones, with the "media" attribute set to "all" or "screen"?
Why only "all" and "screen"? PWA's are mostly used on handheld devices, so why not include 'media="handheld"'?
What does the regex do?
The "e" modifier is used to execute PHP code in the "replace" part of the function, which is obviously very risky and therefore deprecated. But here we use php code for the subject, which is completely fine and shouldn't need the e modifier.I simply removed the "e" modifier for now, but there are still quite a few questions open about this... @Anybody, do you think we need to modify this code further?
Please review.
- Status changed to Needs review
over 1 year ago 8:30am 12 September 2023 - last update
over 1 year ago 9 pass - @grevil opened merge request.
- 🇩🇪Germany Anybody Porta Westfalica
Why are simply not all "style" elements stored? Why only the ones, with the "media" attribute set to "all" or "screen"?
Good question, might be out of scope in this issue. I think they're trying to exclude things like media="printer", which is irrelevant. So it's worth a look, but I think in another issue... and I guess it's hopefully best practice.
Why only "all" and "screen"? PWA's are mostly used on handheld devices, so why not include 'media="handheld"'?
I *think* handheld is outdated and no more used. It's not the same as tablet or smartphone, which has no own type.
So I think let's split the other parts off?
What does the regex do?
and
The "e" modifier is used to execute PHP code in the "replace" part of the function, which is obviously very risky and therefore deprecated. But here we use php code for the subject, which is completely fine and shouldn't need the e modifier.
are the really important parts.
And as I'm also unsure, we need to understand and test to be 100% sure, otherwise we shouldn't merge this.
So you might be right, but you need to proof.
- Status changed to Needs work
over 1 year ago 3:40pm 12 September 2023 - 🇩🇪Germany Anybody Porta Westfalica
@Grevil: Just found an read https://stackoverflow.com/questions/16986331/can-someone-explain-the-e-r... and yes I think the "e" is just wrong.
Let's remove it!
If you think it's worth to keep this task open for the other points (and make it minor), let's remove the e in a separate issue and rename this one! - 🇩🇪Germany Anybody Porta Westfalica
@Grevil: Should we add further subtasks for the listed issues? Or how to proceed best?
- Issue was unassigned.
- 🇩🇪Germany Anybody Porta Westfalica
Workbox might solve some of the issues...
- 🇩🇪Germany Grevil
The service worker is split now in its own submodule, so we can adjust this issue, as all listed issues are specifically service worker issues.