Skip to content
This repository has been archived by the owner on Feb 8, 2020. It is now read-only.

feat: make deep link handling more flexible #97

Merged
merged 1 commit into from
Sep 16, 2019
Merged

Conversation

satya164
Copy link
Member

This adds ability to specify a custom config to control how to convert between state and path.

Example:

{
  Chat: {
    path: 'chat/:author/:id',
    parse: { id: Number }
  }
}

The above config can parse a path matching the provided pattern: chat/jane/42 to a valid state:

{
  routes: [
    {
      name: 'Chat',
      params: { author: 'jane', id: 42 },
    },
  ],
}

This makes it much easier to control the parsing without having to specify a custom function.

This adds ability to specify a custom config to control how to convert between state and path.

Example:

```js
{
  Chat: {
    path: 'chat/:author/:id',
    parse: { id: Number }
  }
}
```

The above config can parse a path matching the provided pattern: `chat/jane/42` to a valid state:

```js
{
  routes: [
    {
      name: 'Chat',
      params: { author: 'jane', id: 42 },
    },
  ],
}
```

This makes it much easier to control the parsing without having to specify a custom function.
@codecov-io
Copy link

Codecov Report

Merging #97 into master will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   93.44%   93.77%   +0.33%     
==========================================
  Files          33       33              
  Lines         671      707      +36     
  Branches      164      179      +15     
==========================================
+ Hits          627      663      +36     
  Misses         38       38              
  Partials        6        6
Impacted Files Coverage Δ
packages/core/src/getPathFromState.tsx 100% <100%> (ø) ⬆️
packages/core/src/getStateFromPath.tsx 100% <100%> (ø) ⬆️

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 d16c20c...591a6df. Read the comment docs.

@jineshshah36
Copy link

What if you need multiple paths to match the same route name?

@brentvatne
Copy link
Member

brentvatne commented Sep 13, 2019

@jineshshah36 - can you explain? this doesn't seem to make sense. if two route names mapped to '/hello' then which route definition would you use?

edit: my bad i read that wrong, it's a valid question

@jineshshah36
Copy link

To clarify, I would expect something like the following:

{
  Chat: [
    {
      path: "chat/:author/:id",
      parse: { id: Number },
    },
    {
      path: "some/:other/:path",
      parse: { path: Number },
    },
  ],
}

@satya164
Copy link
Member Author

satya164 commented Sep 13, 2019

Sounds reasonable. Maybe for a followup PR.

One thing to keep in mind is that in the current PR, this works both ways: you can convert between a state and a path using the same config. If multiple paths can represent the same route, then this mapping isn't 2-way anymore, i.e. we can convert a path to a navigation state, but not the other way around. Perhaps when 2 configs specified, we should treat the first one as the primary config, which will be used for converting state to path.

Why 2-way conversion is needed? Mainly because otherwise we wouldn't be able to support web (I think).

@jineshshah36
Copy link

jineshshah36 commented Sep 13, 2019

That makes sense. I would say the primary reason to have multiple routes match a path is to make it easier to maintain compatibilty when someone changes a path. So, having the first or a “default” marked path take precedence makes sense.

I’d imagine this would be even more important when supporting web since links can’t always all be rewritten if they are on other sites and such. Alternatively, a redirect mechanism in the config could also work.

Copy link
Member

@osdnk osdnk left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes. If you're feeling good with this API, feel free to merge!

@@ -31,7 +31,7 @@
"@types/jest": "^24.0.13",
"codecov": "^3.5.0",
"commitlint": "^8.0.0",
"core-js": "^3.1.4",
"core-js": "^3.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

Can you tell why we need these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@osdnk jest was giving some issue. this is only for tests so not very important.

@satya164 satya164 merged commit 849d952 into master Sep 16, 2019
@satya164 satya164 deleted the @satya164/linking branch September 16, 2019 11:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants