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: support wildcard string prefixes #8942

Merged
merged 7 commits into from Oct 20, 2020

Conversation

hosseinmd
Copy link
Contributor

#8941
Prefixes should be more flexible for situations like wild card subdomain. On android and IOS we can define wild cards by * but react-navigation does not work, In this PR I added support for RegExp Prefixes.

For Example

{
  prefixes: [
    /^[^.s]+.example.com/g
 ],
}

I tested this work well.

@netlify
Copy link

netlify bot commented Oct 8, 2020

Deploy preview for react-navigation-example ready!

Built with commit c4c1f1a

https://deploy-preview-8942--react-navigation-example.netlify.app

@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #8942 into main will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8942      +/-   ##
==========================================
- Coverage   74.45%   74.43%   -0.03%     
==========================================
  Files         131      131              
  Lines        3210     3211       +1     
  Branches      968      968              
==========================================
  Hits         2390     2390              
- Misses        717      718       +1     
  Partials      103      103              
Impacted Files Coverage Δ
packages/native/src/useLinking.native.tsx 54.00% <0.00%> (-1.11%) ⬇️

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 01f86d2...3c576cf. Read the comment docs.

@spacesuitdiver
Copy link

I wonder if we are better of with just a predicate function here instead then we can have whatever logic including your RegExp. Otherwise looks good.

@hosseinmd
Copy link
Contributor Author

hosseinmd commented Oct 17, 2020

I wonder if we are better of with just a predicate function here instead then we can have whatever logic including your RegExp. Otherwise looks good.

Prefixes are defined for telling react-navigation unnecessary parts of the link, react-navigation just watching to routes and query, so host or schema does not matter to them, so as you mentioned we can put there a function which removes unnecessary parts and gives them what matters.
But I need to know what the members think.

@spacesuitdiver
Copy link

spacesuitdiver commented Oct 17, 2020

You’re right, it’s be a filter, we’re on the same page though function seems a little more flexible as you can pull in 3rd party libraries, etc.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I'm not entirely sure if we should support a regex. It makes it more flexible for the user, however there are some issues:

  1. By specifying a regex, it's no longer just a prefix since user can specify a regex to match anything. Maybe it should be a separate option instead like matches or something
  2. Currently we don't do it, but for things like web integration, we would need to be able to understand the prefix, for example, say there's a subpath which we shouldn't overwrite, a regex would prevent us from doing it

Maybe it's better to support what's supported by iOS & Android: e.g. https://*.example.com could work, we would convert it into a regex internally like we do for the paths in the deep link config. It's not as flexible, but if we support everything the underlying platform supports, then it should be fine.

In future, if we have more use case requests, we can consider supporting a matches option for advanced cases.

packages/native/src/types.tsx Outdated Show resolved Hide resolved
packages/native/src/useLinking.native.tsx Outdated Show resolved Hide resolved
Co-authored-by: Satyajit Sahoo <satyajit.happy@gmail.com>
@hosseinmd
Copy link
Contributor Author

Thanks for the PR. I'm not entirely sure if we should support a regex. It makes it more flexible for the user, however there are some issues:

  1. By specifying a regex, it's no longer just a prefix since user can specify a regex to match anything. Maybe it should be a separate option instead like matches or something
  2. Currently we don't do it, but for things like web integration, we would need to be able to understand the prefix, for example, say there's a subpath which we shouldn't overwrite, a regex would prevent us from doing it

Maybe it's better to support what's supported by iOS & Android: e.g. https://*.example.com could work, we would convert it into a regex internally like we do for the paths in the deep link config. It's not as flexible, but if we support everything the underlying platform supports, then it should be fine.

In future, if we have more use case requests, we can consider supporting a matches option for advanced cases.

I agree with you, this pattern (https://*.example.com) is more reliable, and we could document it nicely.

@satya164
Copy link
Member

Can you update the PR to support wildcard string instead of regexes?

@hosseinmd
Copy link
Contributor Author

Can you update the PR to support wildcard string instead of regexes?

Done, please check it.

@hosseinmd hosseinmd changed the title feat: add regexp prefixes feat: support wildcard string prefixes Oct 18, 2020
hosseinmd and others added 2 commits October 18, 2020 20:20
Co-authored-by: Satyajit Sahoo <satyajit.happy@gmail.com>
@hosseinmd
Copy link
Contributor Author

@satya164 I have committed your suggested code with little change, please check 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

4 participants