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

Added ability to specify which URLs should trigger loader #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

engineer-andrew
Copy link

The intention of this change is to allow the user to specify a smaller set of URLs that should trigger the loader; the opposite of the filteredUrlPatterns usage. Using the new includedUrlPatterns parameter overrides the exclusion parameter, but if the includedUrlPatterns parameter is an empty array (the default value) there should be no adverse effects in order to make this a non-breaking change.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1593783833

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1520115633: 0.0%
Covered Lines: 114
Relevant Lines: 114

💛 - Coveralls

@mpalourdio
Copy link
Owner

Thanks for this contribution, that looks interesting! I need to take a good look at it but I'm on holidays.

I understand the semantic of this feature, but why don't you just negate your regex in the filteredUrlPatterns. AFAIK, it will provide this exact behaviour, no ?

@engineer-andrew
Copy link
Author

I hadn't considered that. In my specific use case I only want a few URLs to trigger the loader so it seems like negating the regex would be more costly (doesn't a "negative" regex take longer than a "positive" one to execute)? Either way I'll give it a shot and see if it would meet my needs for kicks and giggles.

@mpalourdio
Copy link
Owner

mpalourdio commented Dec 17, 2021

Negation is not more a perf. hog that a standard evaluation. Greedy repetitions are much more costly (.*). Just negate the regex of URL for which you want the loader to kick in, and you're done IMO.

Also this is javascript at the end, the performance is only dependant of the end user device. And running angular is not something that light :)

@engineer-andrew
Copy link
Author

After some experimentation I believe negating will only work (at least without a very long and confusing regex) if you want to only trigger the loader for a single URL (or single regex anyway). For example, if you negated /users/\d+/cars all calls to \users would be ignored except those calls, which is fine. However, if you wanted the loader to be displayed when the application makes requests to \users{userId}\cars or \cars{carId} and negated both of those ((?!(/users/\d+/cars)) and (?!(/cars/\d+))) all URLs would then be non-negated because calls to \users\123456\cars would be met by the negated (?!(/cars/\d+)) regex.

I'll fully admit that I'm not a regex pro so there may be a very easy solution to this that I'm just missing. From what I've seen so far, even if this is possible it would make the configuration incredibly confusing as you'd be negating URLs via regex that shouldn't be bypassed. I'll leave it to you to determine whether you'd like to incorporate this change into your library, but I still think it's valuable.

Thanks for considering it!

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

3 participants