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

Support for regex in `EffectFactory.matchPath` #67

Closed
zeucxb opened this issue Sep 20, 2018 · 16 comments

Comments

3 participants
@zeucxb
Copy link

commented Sep 20, 2018

Can I use regex in the marble routes?

I know that are some especial chars that I can use like:

EffectFactory
  .matchPath('/:dir*')

But what about more complex regex?

EffectFactory
  .matchPath(/^\/(api|rest)\/.+$/)
@krzysztof-miemiec

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

Internally Marble uses https://github.com/pillarjs/path-to-regexp which supports passing regexp to parameters. However everything has to be written as string. Have you tried .matchPath('/(api|rest)/(.*)')?

@krzysztof-miemiec

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

If you're thinking about something more fancy, like writing actual regular expressions, it's not supported yet, but it's definitely doable. We'd have to think about regex pattern merging for nested routing. It's simple to get regex patterns and concatenate them, but I bet that sooner or later we'll get something like: /my/parent/path/ and /^\/(to-be|not-to-be)\/.+$/ and for any given route it will be simply evaluated as false, because of ^ in the middle. Then we'd have to remove one of the duplicated \/ inside and so on... So, for now I came up with:

  • get both parent and child path parts and simply concatenate them as string
  • remove all ^ and $ from that stringified regex (also using regex, what a perversion 😜 )
  • .replace(([\\/]{2,})/g, '/')
  • somehow extract all capturing groups and assign them to parameters; What should we do with groups that were defined in regex? There is a specification for named capturing groups that we could use, but AFAIK they haven't landed in JS. We can simply mark all groups from regex as non-capturing, but this has to be done earlier, before concatenation.
  • append ^ and $ once again
  • probably check for possibility of hacks on such routing mechanism

Since Marble is one of the milion solutions available, the above is probably redundant and someone might have implemented such merger.

@zeucxb zeucxb closed this Sep 21, 2018

Future ideas automation moved this from To do to Done Sep 21, 2018

@JozefFlakus JozefFlakus moved this from Done to To do in Future ideas Sep 21, 2018

@JozefFlakus JozefFlakus moved this from To do to Done in Future ideas Sep 21, 2018

@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@zeucxb have you tested it with stringified Regex? 👇
#67 (comment)

@zeucxb zeucxb reopened this Sep 21, 2018

Future ideas automation moved this from Done to In progress Sep 21, 2018

@JozefFlakus JozefFlakus moved this from In progress to To do in Future ideas Sep 21, 2018

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

Sorry, I have tried in the version that was running in the project here.

But checking the urlParams.factory.ts it should have worked. So I created a new project with the current stable version and it works.

👎 Sorry 👎

@zeucxb zeucxb closed this Sep 21, 2018

Future ideas automation moved this from To do to Done Sep 21, 2018

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

But we have a problem if try to use regex and special params.

ex.

matchPath: /(api|rest)/:file

request: /rest/test.json

params: { file: "rest" }

@JozefFlakus JozefFlakus reopened this Sep 21, 2018

Future ideas automation moved this from Done to In progress Sep 21, 2018

@JozefFlakus JozefFlakus added the bug label Sep 21, 2018

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

And without the special param (only with regex) we cannot get url params.

@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

re: #67 (comment)
Can you post also the example regex / path that was used within matchPath?

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

@JozefFlakus this?

With :file

matchPath: /(api|rest)/:file

request: /rest/test.json

params: { file: "rest" }

Without :file

matchPath: /(api|rest)/(.*)

request: /rest/test.json

params: { }
@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Thanks. Custom regex is problematic because params are mapped over declared names in order they occured, eg. /(api|rest)/:file has a one named parameter, which in result will be mapped to the first capturing grup. Maybe we should mach it in more precise way @krzysztof-miemiec? 🤔

@krzysztof-miemiec

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@zeucxb I think you can try with a named param there, i.e.: matchPath: /:type(api|rest)/:file, then you should get {type: "api", file: "test.json"} as a result for /api/test.json.

The naming is probably a bug on our side, as path-to-regexp docs state that:

It is possible to write an unnamed parameter that only consists of a matching group. It works the same as a named parameter, except it will be numerically indexed.

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

Yeah @krzysztof-miemiec it works. Awesome!!!

@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

So far this kind of advanced routing is not documented and wasn't tested before. We should cover these features with test cases first. Thanks @zeucxb for pointing this out.

@zeucxb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

I was about to say that we should have routes' documentation. 😄

@zeucxb

This comment has been minimized.

Copy link
Author

commented Oct 17, 2018

I was going to open another issue but I don't know if it's an expected behavior.

I've this code in my application:

const web$ = combineRoutes("/admin", [
  EffectFactory
    .matchPath('/:dir')
    .matchType('GET')
    .use((req$) => ...), 
  EffectFactory
    .matchPath('*')
    .matchType('GET')
    .use((req$) => ... ),
]);

In this case a request to /admin gets 404 but /admin/ works fine.

If I change the code to:

const web$ = combineRoutes("/admin", [
  EffectFactory
    .matchPath('/:dir')
    .matchType('GET')
    .use((req$) => ...), 
  EffectFactory
    .matchPath('/')
    .matchType('GET')
    .use((req$) => ... ),
]);

Both /admin and /admin/ works.

@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

It is a bug. @zeucxb please create a separate issue so we can track it properly :)

@JozefFlakus

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

@zeucxb fixed with v1.1.1 patch 💪

@JozefFlakus JozefFlakus removed the bug label Oct 24, 2018

@JozefFlakus JozefFlakus changed the title Route regex. Add support for regex in `EffectFactory.matchPath` Oct 24, 2018

@JozefFlakus JozefFlakus changed the title Add support for regex in `EffectFactory.matchPath` Support for regex in `EffectFactory.matchPath` Oct 24, 2018

@JozefFlakus JozefFlakus moved this from In progress to To do in Future ideas Oct 27, 2018

@JozefFlakus JozefFlakus closed this Nov 6, 2018

Future ideas automation moved this from To do to Done Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.