fix(remix-dev): allow defining multiple routes for the same route module file #3970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 86b8720 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
lucasferreira
changed the title
lucasferreira
changed the title
lucasferreira
changed the title
Co-authored-by: Andrew Leedham <[email protected]>
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
- Better name for automated ID variable; - Small adjust in `id` option comment;
|
I made a small change in the last commit in variable What is the next step to me? |
The changes looks good to me. Unfortunately I'm not a maintainer, just an interested party, so the PR getting the attention and subsequent review of a maintainer would be the next step I would have thought. |
|
Nice work! I think it would be good if the docs are also updated, but maybe this can come later once the idea has been given the OK |
I agree with the future docs update and if someone need some light code sample to test this PR: module.exports = {
appDirectory: "app",
assetsBuildDirectory: "public/build",
devServerPort: 8002,
publicPath: "/build/",
serverBuildDirectory: "build",
ignoredRouteFiles: [".*"],
routes(defineRoutes) {
return defineRoutes(route => {
route("/user/:id", "routes/index.tsx", { id: "user-by-id" });
route("/user", "routes/index.tsx");
});
}
}; |
|
@lucasferreira what's the easiest way to test this logic in my own app? |
I know thats weird but when I started develop this changes I worked directly inside de https://github.com/remix-run/remix/pull/3970/files Basically you need to change 2 files But if you need to test isolated some code and sample project you could checkout my modified branch (https://github.com/lucasferreira/remix/tree/dev) and create some playground app: https://github.com/remix-run/remix/blob/main/docs/pages/contributing.md#playground |
|
Just played around with this locally, and it works a treat! Would be great to get this merged in :) @runmoore I got this working by making a patch-package patch, I'll include it below.
|
brophdawg11
reviewed
Sep 22, 2022
| groupedRoute, | ||
| `Cannot get route(s) for entry point ${output.entryPoint}` | ||
| ); | ||
| if (groupedRoute) { |
There was a problem hiding this comment.
I don't think this if is needed due to the above invariant
There was a problem hiding this comment.
Hi @brophdawg11
You're right, I will remove this IF.
brophdawg11
reviewed
Sep 22, 2022
brophdawg11
reviewed
Sep 22, 2022
packages/remix-dev/config/routes.ts
Outdated
| let route: ConfigRoute = { | ||
| path: path ? path : undefined, | ||
| index: options.index ? true : undefined, | ||
| caseSensitive: options.caseSensitive ? true : undefined, | ||
| id: createRouteId(file), | ||
| id: options.id ? `${fileId}--${options.id}` : fileId, |
There was a problem hiding this comment.
what is the best way to "verbatim and error if they provide duplicates" just throws a new Error inside routes.ts file?
Maybe just change this to
Suggested change
| id: options.id ? `${fileId}--${options.id}` : fileId, | |
| id: options.id || createRouteId(file), |
And delete line 147 above
brophdawg11
reviewed
Sep 22, 2022
|
Thanks @AndrewLeedham for sharing the patch solution. @lucasferreira how does this work with nested routes? I'm trying the below where the use case it to map a translated url back to the 'original' one in english which is in the file system. In this example I get just the nested content routes(defineRoutes) {
return defineRoutes((route) => {
route('/vols', 'routes/flights/index.tsx', { id: 'flights-fr-fr' });
});
},and in this case I just get the 'outside' content, with none of the nesting. routes(defineRoutes) {
return defineRoutes((route) => {
route('/vols', 'routes/flights.tsx', { id: 'flights-fr-fr' });
});
},There's a brief explanation in the documentation for nesting, but I can't make sense of it. |
|
@runmoore The third parameter to the routes(defineRoutes) {
return defineRoutes((route) => {
route('/vols', 'routes/flights.tsx', { id: 'flights-fr-fr' }, () => {... child routes });
});
}, |
|
@kiliman like this? I've tested it locally, and it works, but the nested routing is really confusing me. routes(defineRoutes) {
return defineRoutes((route) => {
route(
'/vols', 'routes/flights.tsx', { id: 'flights-root-fr-fr' },
() => {
route('/vols', 'routes/flights/index.tsx', { id: 'flights-fr-fr' });
route('/vols/a-propos', 'routes/flights/about/index.tsx', { id: 'flights-about-fr-fr' },
);
},
);
});
}, |
|
With nested routes, the path should be relative to its parent route. Also specify routes(defineRoutes) {
return defineRoutes((route) => {
route(
'vols', 'routes/flights.tsx', { id: 'flights-root-fr-fr' },
() => {
route('', 'routes/flights/index.tsx', { id: 'flights-fr-fr', index: true });
route('a-propos', 'routes/flights/about/index.tsx', { id: 'flights-about-fr-fr' },
);
},
);
});
},<Routes>
<Route file="root.tsx">
<Route index file="routes/index.tsx" />
<Route path="vols" file="routes/flights.tsx">
<Route index file="routes/flights/index.tsx" />
<Route path="a-propos" file="routes/flights/about/index.tsx" />
</Route>
</Route>
</Routes> |
|
Thanks @kiliman, this is very useful, especially about the undocumented |
Trying to solve a conflict
This reverts commit 2064c57.
Hi @brophdawg11 and @AndrewLeedham , I followed the indicated instructions here in my last commit, now the custom ID it's 100% customizable and we are checking for dupes inside But now we have some conflict in @brophdawg11 could you help us in that? |
Could you add remix as an upstream remote locally and do a git merge or rebase? |
Hi @AndrewLeedham I really don't know how to do that 😫 I'm working only in my fork in dev branch, is it easy to find how to do this rebase thing? I'm not git expert =/ |
Hi @lucasferreira. I would usually do this from the command-line, if you are using a GUI tool there may be a way to do it there. The command-line way is as follows:
Hope that helps 😃 |
|
That's was more easy than expected! Thank you very much @AndrewLeedham! |
brophdawg11
reviewed
Nov 10, 2022
| @@ -95,18 +95,72 @@ describe("defineRoutes", () => { | |||
| route("/other", "routes/other-route.tsx"); | |||
| }); | |||
|
|
|||
| expect(Object.entries(routes)).toHaveLength(3); | |||
There was a problem hiding this comment.
Made this test a bit stronger by asserting that we're getting the right paths, files, and IDs on defined routes 👍
brophdawg11
reviewed
Nov 10, 2022
| throw new Error( | ||
| `You tried to define a route with custom id "${options.id}" but one ` + | ||
| "already exists. Custom route ids must be unique" | ||
| `Unable to define routes with duplicate route id: "${route.id}"` |
There was a problem hiding this comment.
I adjusted this to account for the (very edge-case) scenario that an auto-generated route id collies with a previously-defined custom id and expanded the tests above to assert the new behavior
brophdawg11
approved these changes
Nov 10, 2022
|
Hi @brophdawg11 anything left for me to do here and help this PR? |
jacob-ebey
approved these changes
Nov 29, 2022
|
@lucasferreira Do you mind adding a changeset? There are instructions in #3970 (comment) but the gist of it is run |
Done! |
|
Thanks @lucasferreira! |
|
Thanks for your work on this @lucasferreira! Is there an ETA for when this'll be released? |
|
@constantincerdan Probably next week, or the one after it. |
This was referenced Dec 13, 2022
pcattori
added a commit
that referenced
this pull request
Dec 16, 2022
* fix(remix-dev): convert `config.appDirectory` to relative unix path (#4709) * fix(remix-dev): convert appDirectory to unix style for fast-glob Signed-off-by: Logan McAnsh <[email protected]> * chore: relative path Signed-off-by: Logan McAnsh <[email protected]> * chore: add test Signed-off-by: Logan McAnsh <[email protected]> * fix test Signed-off-by: Logan McAnsh <[email protected]> * fix: typo Signed-off-by: Logan McAnsh <[email protected]> * chore: update test Signed-off-by: Logan McAnsh <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> * chore: add changeset for #4709 (#4718) * ci: add typechecking for deno (#4715) * ci: add typechecking for deno * ci: install deno for integration tests * chore: format * chore: format * fix: Firefox LiveReload (#4725) Firefox infinitely reloads the page as long as `<LiveReload>` is rendering. Closes #4692 * fix(remix-dev): allow defining multiple routes for the same route module file (#3970) * Allow multiple routes for same route module * Update packages/remix-dev/config/routes.ts Co-authored-by: Andrew Leedham <[email protected]> * Update routes.ts - Better name for automated ID variable; - Small adjust in `id` option comment; * - Removing redundant IF * Update routes.ts Revert complex custom ID in routes * Non unique custom routes ID error and test * Update assets.ts Trying to solve a conflict * Revert "Update assets.ts" This reverts commit 2064c57. * Error on collisions with non-custom routeIds * Create big-spoons-grab.md Co-authored-by: Andrew Leedham <[email protected]> Co-authored-by: Matt Brophy <[email protected]> * feat: Allow pass-through script props in `ScrollRestoration` (#2879) * ci(nightly): add deno for typechecking deno files (#4738) * ci: fix race condition writing globals.d.ts shim (#4717) Co-authored-by: Chance Strickland <[email protected]> * chore: bump @playwright/test to latest (#4749) Signed-off-by: Logan McAnsh <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> * Fix 4199: TypedResponse allows incompatible types (#4734) * Fixes #4199: Do not allow assignment of incompatible TypedResponses * Add myself to contributors.yml * Create light-sheep-give.md * slight changeset tweak * additional changeset tweaks Co-authored-by: Pedro Cattori <[email protected]> * chore: format * test: add transition integration tests (#4739) test: useTransition to wait for states This approach could probably be applied across other flakey tests and could also be documented as a good approach of if there is user-feedback for a specific action when running integration tests. * feat: testing helpers (#4539) Co-authored-by: James Restall <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> * chore: format * chore(remix-testing): update dependencies (#4756) * fix(remix-testing): fix deps (#4757) * Fix deps for new remix-testing package * fix lint * Remove ENABLE_REMIX_ROUTER flags (#4732) * chore: add `@remix-run/testing` to changesets (#4781) * chore: add `@remix-run/testing` to changesets Signed-off-by: Logan McAnsh <[email protected]> * chore: update `ADDING_A_PACKAGE.md` Signed-off-by: Logan McAnsh <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> * refactor(remix-react): upgrade Remix to `[email protected]` (non-data-router) and drop `history` (#4731) * Bump remix to [email protected] (#4668) Co-authored-by: Mehdi Achour <[email protected]> * Bump remix to RR 6.4.4 and drop history dependency (#4702) Co-authored-by: Mehdi Achour <[email protected]> * ci(nightly): move git operations after build (#4797) * ci(nightly): move git operations after build when adding deno typechecking (#4715), nightly now refers to the version we just calculated which hasnt been published when trying to build and typecheck Signed-off-by: Logan McAnsh <[email protected]> * ci: update tmp branch name Signed-off-by: Logan McAnsh <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> * perf(remix-dev): Optimize `parentRouteId` lookup in `defineConventionalRoutes` (#4538) * Use object for parentRouteId lookup * Sign CLA * Move parentRouteId logic to a separate function * Update test fixture path * chore: format * chore(dev): add changeset for PR #4538 (#4800) * chore: format * refactor(remix-react): use `react-router-dom` import instead of `react-router` (#3325) * chore: manually bump remix-testing version due to not being on main just yet Signed-off-by: Logan McAnsh <[email protected]> * fixup! chore: manually bump remix-testing version due to not being on main just yet * fixup! chore: manually bump remix-testing version due to not being on main just yet Signed-off-by: Logan McAnsh <[email protected]> * chore: unify error usage (#4696) * chore: format * Add fetcher state/type tests (#4803) * chore(deps): bump esbuild to latest (#4754) * chore: add invariant instead of using `!` Signed-off-by: Logan McAnsh <[email protected]> * chore(deps): bump esbuild to latest Signed-off-by: Logan McAnsh <[email protected]> * Create fresh-shrimps-join.md Signed-off-by: Logan McAnsh <[email protected]> Co-authored-by: Pedro Cattori <[email protected]> * test(integration): close server synchronously (#4785) * chore: normalize afterAll Signed-off-by: Logan McAnsh <[email protected]> * test: close server synchronously Signed-off-by: Logan McAnsh <[email protected]> * chore: appFixture.close isnt async anymore Signed-off-by: Logan McAnsh <[email protected]> * Update integration/helpers/create-fixture.ts Co-authored-by: Pedro Cattori <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> Co-authored-by: Pedro Cattori <[email protected]> * feat: remix optional segments (#4706) * feat: transform optional routes from remix to react router * Add to contributors * Add changeset * fix(optional-segments): fix escaping of parenthesis * small function fix * Update packages/remix-dev/__tests__/routesConvention-test.ts Co-authored-by: Pedro Cattori <[email protected]> Co-authored-by: Pedro Cattori <[email protected]> * chore: format * chore: edit the optional segments changeset (#4815) to make it clear that Remix won't support optional segments until integrated with React Router 6.5 * chore: format * docs: rearrange docs, give everything its own page (#4821) * chore: format * fix: wrong parentheses in the optional segments changeset (#4825) * fix: wrong parentheses in the optional segments changeset * add: akamfoad to the contributers * ci(nightly): add workflow_call for testing purposes Signed-off-by: Logan McAnsh <[email protected]> * Revert "ci(nightly): add workflow_call for testing purposes" This reverts commit f3fa77e. * chore(scripts): Use relaxed peer dependencies to avoid triggering major version bumps (#4736) - Add script to bump peer deps with changesets version Co-authored-by: Mateusz Burzyński <[email protected]> * Add integration tests for request structures (#4829) * fix(remix-dev): resolve asset entry full path to support mono-repo import of styles (#4855) * chore: have eslint report unused eslint comments (#4863) * chore: have eslint report unused eslint comments Signed-off-by: Logan McAnsh <[email protected]> * chore: remove additional comment Signed-off-by: Logan McAnsh <[email protected]> * perf(remix-architect,remix-netlify): improve performance of `isBinaryType` (#4761) Co-authored-by: Logan McAnsh <[email protected]> Co-authored-by: Pannatier Guillaume <[email protected]> * chore: format * chore(remix-testing): remove internal installGlobals (#4755) * chore: remove internal installGlobals Signed-off-by: Logan McAnsh <[email protected]> * chore: add README Signed-off-by: Logan McAnsh <[email protected]> * Create quick-cats-fix.md * Update .changeset/quick-cats-fix.md Co-authored-by: Michaël De Boey <[email protected]> * chore(deps): remove jsdom and happydom from devDependencies Signed-off-by: Logan McAnsh <[email protected]> Signed-off-by: Logan McAnsh <[email protected]> Co-authored-by: Michaël De Boey <[email protected]> * fix(dev): build js modules for ts->js conversion The TS->JS migration was removed from the CLI codemod options, but still used for TS->JS conversion when creating a new Remix project from the CLI. The TS modules responsible for the TS->JS conversion were incorrectly removed from the Rollup build, resulting in the corresponding built JS modules being absent. That caused the CLI to error when trying to perform TS->JS conversion. This changes reintroduces the wiring to build the modules responsible for the TS->JS conversion. Fixes #4854 Signed-off-by: Logan McAnsh <[email protected]> Co-authored-by: Logan McAnsh <[email protected]> Co-authored-by: Matt Kane <[email protected]> Co-authored-by: Remix Run Bot <[email protected]> Co-authored-by: Chance Strickland <[email protected]> Co-authored-by: Ryan Florence <[email protected]> Co-authored-by: Lucas Ferreira <[email protected]> Co-authored-by: Andrew Leedham <[email protected]> Co-authored-by: Matt Brophy <[email protected]> Co-authored-by: dabdine <[email protected]> Co-authored-by: Jacob Ebey <[email protected]> Co-authored-by: James Restall <[email protected]> Co-authored-by: Michaël De Boey <[email protected]> Co-authored-by: Mehdi Achour <[email protected]> Co-authored-by: Dylan Markow <[email protected]> Co-authored-by: Daniel Rios <[email protected]> Co-authored-by: Akam Foad <[email protected]> Co-authored-by: Mateusz Burzyński <[email protected]> Co-authored-by: Guillaume Pannatier <[email protected]> Co-authored-by: Pannatier Guillaume <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #1898
Testing Strategy:
I copied a failing test to show that multiple routes pointing at a single route module get overwritten from here #3684 and works in some files to get this works for real.
Basically I've changed the
remix-dev/config/routes.tsto allow receive some optional custom ID for routes because in a normal way two or more routes to the same route module will receive internally the same ID, so if somebody could pass a new unique ID we will resolve this this "route overwritten" situation.Also to get the this idea completely working I've had too changed
remix-dev/compiler/assets.tsto allow the compiler groups more then one route per route module file.