Skip to content

Commit

Permalink
chore(sagaRouter): upgrade path-to-regexp (#5480)
Browse files Browse the repository at this point in the history
  • Loading branch information
yyanwang authored Dec 31, 2024
1 parent c156674 commit 816bb91
Show file tree
Hide file tree
Showing 8 changed files with 262 additions and 71 deletions.
35 changes: 35 additions & 0 deletions .changeset/breezy-ways-care.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
'@talend/react-cmf-router': major
'@talend/react-cmf': major
---

BREAKING CHANGE: Upgraded path-to-regexp from 3.x to 8.x

This upgrade was necessary to resolve security vulnerabilities. The new version introduces two breaking changes that require updates to your application:

1. Optional Path Parameter Syntax Change
- Old syntax: `/resources/:id?`
- New syntax: `/resources{/id}`

This change is required because in path-to-regexp 8.x, the `?` character is reserved for query parameters and will throw a parsing error when used at the end of a path.

2. Root Path Matching Behavior Change
- In v3.x, root path `/` would match any path starting with `/`
- In v8.x, root path `/` only matches exactly `/`
- To match both root and child paths, use the wildcard pattern `/{*path}`

Example migration:
```javascript
// Before
const routes = {
'/': rootSaga,
'/resources/:id?': resourceSaga
};

// After
const routes = {
'/{*path}': rootSaga, // if you want to match all routes
'/resources{/id}': resourceSaga
};
```
For more details about path matching and troubleshooting, see [path-to-regexp documentation](https://github.com/pillarjs/path-to-regexp#errors).
2 changes: 1 addition & 1 deletion packages/cmf-router/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"connected-react-router": "^6.9.3",
"history": "^5.3.0",
"lodash": "^4.17.21",
"path-to-regexp": "^3.3.0",
"path-to-regexp": "^8.2.0",
"prop-types": "^15.8.1",
"react-redux": "^7.2.9",
"react-router": "~6.3.0",
Expand Down
149 changes: 98 additions & 51 deletions packages/cmf-router/src/sagaRouter.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ yield spawn(routerSaga, history, routes);
```

## Matching pattern

```javascript
const routes = {
"/datasets/add": saga1,
"/connections/:datastoreid/edit/add-dataset": saga2
'/datasets/add': saga1,
'/connections/:datastoreid/edit/add-dataset': saga2,
};
```

Expand All @@ -65,9 +66,9 @@ and that we have the following configuration

```javascript
const routes = {
"/datasets": datasets,
"/datasets/add": datasetsSaga,
"/connections/add": connectionsSaga
'/datasets': datasets,
'/datasets/add': datasetsSaga,
'/connections/add': connectionsSaga,
};
```

Expand All @@ -90,10 +91,11 @@ and that we have the following configuration

```javascript
const routes = {
"/datasets/add": datasetsSaga,
"/connections/add": connectionsSaga
'/datasets/add': datasetsSaga,
'/connections/add': connectionsSaga,
};
```

only `datasetsSaga` will be executed.

Now the route is changed to `localhost/connections/add`
Expand All @@ -116,35 +118,34 @@ so saga of `/datasets` will be restarted when route changes.
Optionally, if you want to run a saga only on exact match, you can pass a configuration `runOnExactMatch` as true,
then saga will be started when its route exactly match current location, and will be stopped when change to any other route.


```javascript
import { sagaRouter } from '@talend/react-cmf';
import { browserHistory as history } from 'react-router';

import { sagaRouter } from '@talend/react-cmf';

const CANCEL_ACTION = 'CANCEL_ACTION';
// route configuration, a url fragment match with a generator
const routes = {
'/datasets': {
// runOnExactMatch: true,
restartOnRouteChange: true,
saga: function* datasets(notUsed, isExact) {
if (!isExact) {
return;
}
yield take(CANCEL_ACTION);
yield put({
type: REDIRECT_CONNECTION_ADD_DATASET_CANCEL,
cmf: {
routerReplace: `/connections/${datastoreId}/edit`,
},
});
},
},
'/datasets/add': function* addDataset() {}
'/datasets': {
// runOnExactMatch: true,
restartOnRouteChange: true,
saga: function* datasets(notUsed, isExact) {
if (!isExact) {
return;
}
yield take(CANCEL_ACTION);
yield put({
type: REDIRECT_CONNECTION_ADD_DATASET_CANCEL,
cmf: {
routerReplace: `/connections/${datastoreId}/edit`,
},
});
},
},
'/datasets/add': function* addDataset() {},
};
```


### Partial route matching

Given the webapp url is `localhost/datasets/add/connection/add`
Expand All @@ -153,10 +154,11 @@ and that we have the following configuration

```javascript
const routes = {
"/datasets/add": datasetsSaga,
"/connections/add": connectionsSaga
'/datasets/add': datasetsSaga,
'/connections/add': connectionsSaga,
};
```

only `datasetsSaga` will be executed.

because the route key can be matched on any part of the url.
Expand All @@ -171,57 +173,61 @@ and that we have the following configuration

```javascript
const routes = {
"/datasets/add": datasetsSaga,
"/datasets/add/connection/add": datasetConnectionsSaga,
"/connection/add": connectionsSaga,
'/datasets/add': datasetsSaga,
'/datasets/add/connection/add': datasetConnectionsSaga,
'/connection/add': connectionsSaga,
};
```

`datasetsSaga`, `datasetConnectionSaga` and `connectionSaga` are running.

### Route matching and route parameters.

Given the webapp url is `localhost/datasets/50/edit`

and that we have the following configuration

```javascript
function* editDatasetSaga ({datasetId}){
// do something
function* editDatasetSaga({ datasetId }) {
// do something
}

const routes = {
"/datasets/add": datasetsSaga,
"/datasets/:datasetId/edit": editDatasetSaga
'/datasets/add': datasetsSaga,
'/datasets/:datasetId/edit': editDatasetSaga,
};
```

only `editDatasetsSaga` will be executed and :datasetId will be resolved and given to the running saga as a parameter.

url parameters are resolved and given to the executed saga in form of an object, because we can match on many of them.

```javascript
function* connectionSaga ({connectionId, datasetId}){
// do something
function* connectionSaga({ connectionId, datasetId }) {
// do something
}

const routes = {
"/datasets/add": datasetsSaga,
"/datasets/:datasetId/edit": editDatasetSaga,
"/datasets/:datasetId/connections/:connectionId": connectionSaga
'/datasets/add': datasetsSaga,
'/datasets/:datasetId/edit': editDatasetSaga,
'/datasets/:datasetId/connections/:connectionId': connectionSaga,
};
```

### Route matching with route parameter change

Given the webapp url is `localhost/datasets/50/edit`

and that we have the following configuration

```javascript
function* editDatasetSaga ({datasetId}){
// do something
function* editDatasetSaga({ datasetId }) {
// do something
}

const routes = {
"/datasets/add": datasetsSaga,
"/datasets/:datasetId/edit": editDatasetSaga
'/datasets/add': datasetsSaga,
'/datasets/:datasetId/edit': editDatasetSaga,
};
```

Expand All @@ -234,24 +240,65 @@ the `editDatasetsSaga` is cancelled, and when its done, restarted with the new v
Only sagas matching on a route which parameter change are restarted.

### Route matching with optionnal parameters

Given the webapp url is `localhost/datasets/add/550`

and that we have the following configuration

```javascript
function* editDatasetSaga ({datasetId}){
// do something
function* editDatasetSaga({ datasetId }) {
// do something
}

const routes = {
"/datasets/add/:connectionId?": datasetsSaga,
"/datasets/:datasetId/edit": editDatasetSaga
'/datasets/add{/:connectionId}': datasetsSaga,
'/datasets/:datasetId/edit': editDatasetSaga,
};
```

datasetSaga will be executed

if the route change to `localhost/datasets/add`

the `datasetsSaga` will be restarted since it still match on `/datasets/add/:connectionId?` route and that the parameter has changed from being a value to being absent.
the `datasetsSaga` will be restarted since it still match on `/datasets/add{/:connectionId}` route and that the parameter has changed from being a value to being absent.

the {/:connectionId} at the end of path means /connectionId is optional.

### Root Path Matching

The root path `/` has special matching behavior that's important to understand:

1. Exact root path matching:

```javascript
const routes = {
'/': function* rootSaga() {
yield take('SOMETHING');
},
};
```

- Only matches exactly `/`
- Does not match child routes like `/tasks` or `/users/123`
- This is because path-to-regexp treats the root path `/` differently than other routes - it won't do partial matching even when `exact` is false
- If you want `/` to match any path that starts with `/`, you need to use a wildcard pattern like `/{*path}`

2. Matching root and all child routes:

```javascript
const routes = {
'/{*path}': function* rootSaga({ path }) {
yield take('SOMETHING');
},
};
```

- Matches both root path `/` and all child routes
- For root path `/`, params will be empty `{}`
- For child routes, `params.path` will contain the remaining path:
- `/tasks``{ path: 'tasks' }`
- `/tasks/123``{ path: 'tasks/123' }`

This pattern is particularly useful when you need a saga to run for all routes in your application while still being able to access the current route path.

the ? at the end of the parameter define that it is optional.
For more details about path matching and troubleshooting, see [path-to-regexp documentation](https://github.com/pillarjs/path-to-regexp#errors).
65 changes: 64 additions & 1 deletion packages/cmf-router/src/sagaRouter.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { spawn, take, cancel } from 'redux-saga/effects';
import { createMockTask } from '@redux-saga/testing-utils';
import { cancel, spawn, take } from 'redux-saga/effects';

import sagaRouter from './sagaRouter';

describe('sagaRouter import', () => {
Expand Down Expand Up @@ -356,4 +357,66 @@ describe('sagaRouter route and route params', () => {
spawn(routes['/matchingroute/:id'], { id: 'anotherId' }, true),
);
});

it('should handle optional route parameters', () => {
const mockTask = createMockTask();
function getMockedHistory() {
let count = 0;
return {
get location() {
if (count === 0) {
count = 1;
return {
pathname: '/matchingroute/tasks/taskId-123',
};
}
return {
pathname: '/matchingroute/tasks',
};
},
};
}
const routes = {
'/matchingroute/:resource{/:optional}': function* matchingSaga() {
yield take('SOMETHING');
},
};
const gen = sagaRouter(getMockedHistory(), routes);

expect(gen.next().value).toEqual(
spawn(
routes['/matchingroute/:resource{/:optional}'],
{ resource: 'tasks', optional: 'taskId-123' },
true,
),
);
expect(gen.next(mockTask).value).toEqual(take('@@router/LOCATION_CHANGE'));

// optional parameter is removed, saga should be restarted
const expectedCancelYield = cancel(mockTask);
expect(gen.next({ type: '@@router/LOCATION_CHANGE' }).value).toEqual(expectedCancelYield);
expect(gen.next().value).toEqual(
spawn(routes['/matchingroute/:resource{/:optional}'], { resource: 'tasks' }, true),
);
});

it('should start root path saga when on child route', () => {
const mockHistory = {
get location() {
return {
pathname: '/tasks',
};
},
};
const routes = {
'/{*path}': function* rootSaga() {
yield take('SOMETHING');
},
};
const gen = sagaRouter(mockHistory, routes);

// Root saga should be started with isExact=true for wildcard path
expect(gen.next().value).toEqual(spawn(routes['/{*path}'], { path: 'tasks' }, true));
expect(gen.next().value).toEqual(take('@@router/LOCATION_CHANGE'));
});
});
Loading

0 comments on commit 816bb91

Please sign in to comment.