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

drop new RegExp() in favor of generating the javascript regex #144

Closed
tacman opened this issue Mar 19, 2024 · 8 comments · Fixed by #149
Closed

drop new RegExp() in favor of generating the javascript regex #144

tacman opened this issue Mar 19, 2024 · 8 comments · Fixed by #149
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@tacman
Copy link
Contributor

tacman commented Mar 19, 2024

Description

Instead of

// RegexMatchCallbackHandler.php

        return sprintf("new RegExp('%s')", trim(mb_substr($matchCallback, 6)));

// use 
        return trim(mb_substr($matchCallback, 6));

When the regex is wrong, you'll see the error right away. Plus, you get flags (which we don't currently support with regex: ) and it's easier for PHP programmers to understand, even though it's technically a javascript regex.

Some musings at Spomky-Labs/phpwa-doc#12

Example

No response

@Spomky
Copy link
Member

Spomky commented Mar 19, 2024

Hi,

If you just set a string without any recognized pattern, the raw string is used as it is.
I do not know the advantages of the new RexExp('foo') compared to just 'foo'.

@tacman
Copy link
Contributor Author

tacman commented Mar 19, 2024

I think the only advantage of creating a RegExp is when the string is dynamic;

const myRegex = 'ab+c';
new RegExp(myRegex);

but since the bundle is generating javascript, we don't need to do that, we can simply generate the regex as javascript.

One big advantage to this approach is that if the user makes an error in the pwa.yaml file, they will see the error immediately in the javascript console. Otherwise, we should compile the expression and tell the user.

I found several errors in my pwa.yaml file, mostly because I was confused about the regex pattern.

@tacman
Copy link
Contributor Author

tacman commented Mar 19, 2024

Unlike PHP, it appears that you can't change the regex markers, and since we're dealing a lot with slashes, maybe it's easier to use RegExp(). We can add something for the flags if we have a good use case (e.g. matching JPG and PNG files instead of .jpg and .png).

Instinctively, seeing regex: /article looks like a bad regex to me. What if this key were named RegExp instead? That would make it clear it was a javascript RegExp and not php.

@Spomky
Copy link
Member

Spomky commented Mar 19, 2024

The simpler, the better.

We can add something for the flags if we have a good use case

Nothing prevent users to directly use JS

match_callback: 'new RegExp("/foo/bar", "g")'

Maybe I should simply drop it in favor of raw values.

@Spomky Spomky self-assigned this Mar 19, 2024
@Spomky Spomky added the good first issue Good for newcomers label Mar 19, 2024
@Spomky Spomky added this to the 1.1.0 milestone Mar 19, 2024
@tacman
Copy link
Contributor Author

tacman commented Mar 19, 2024

I'm still a bit confused, some examples will help. I've got a few projects using pwa-bundle, I'd like them all to be working by the time you do your talk.

Running a Symfony app on your phone without an internet connection and without writing a single line of javascript sounds like science fiction, but it's sooo close!

@Spomky Spomky linked a pull request Mar 21, 2024 that will close this issue
4 tasks
@Spomky
Copy link
Member

Spomky commented Mar 21, 2024

I added a section in the documentation.
I hope it will clarify the match_callback option.
Also, I removed the regex: handler as it is ultimately useless.

@Spomky
Copy link
Member

Spomky commented Mar 21, 2024

Running a Symfony app on your phone without an internet connection and without writing a single line of javascript sounds like science fiction, but it's sooo close!

I hope this will allow users to adopt the PWA standard. Developers will focus on the application and not on the implementation details.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants