-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: update flat-route module check for file colocation #5034
Conversation
🦋 Changeset detectedLatest commit: 558a448 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
81c34cd
to
a3bc9e6
Compare
Signed-off-by: Logan McAnsh <[email protected]> chore: update regex Signed-off-by: Logan McAnsh <[email protected]> test: add "path" to all tests Signed-off-by: Logan McAnsh <[email protected]> chore: update regex Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]> chore: update regex Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]> test: add explicit id to tests Signed-off-by: Logan McAnsh <[email protected]>
0ab2f88
to
f3eda39
Compare
let routeRegex = | ||
/(([+][/\\][^/\\:?*]+)|[/\\]((_index)|([^/\\:?*])))\.(tsx?|jsx?|mdx?)$$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is too complex for me to understand at a glance. Could we break this down into simpler pieces somehow?
For example, this regex matches /a.js
, but not /abc.js
nor a.js
. I'm not really sure if that's intentional or not.
It also seems to only match after the final /
so /abc/d.js
is only matched on /d.js
. If we only want to match after the final path separator, I think it'd be better to use path
utilities to get the base filename first and then match against regexes.
In any case, I think this deserves at least a comment explaining it, but probably warrants doing it a different way to keep things readable/understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the regex is complex. Even more so in my package as it handles more conventions. Unfortunately, JavaScript doesn't support the x
flag (ignore whitespace) to make the regex easier to read.
Since routes can be determined as either a filename a.b.c.tsx
or a folder a.b.c./_index.tsx
, I don't think you can just get the base filename.
The filepath in isRouteModuleFile
will be the full path after routes
folder. So will never start with /
.
To match flat-files
convention, this is the regex: ^[^\/\\:?*]+\.(tsx?|jsx?|mdx?)$
- Start of path
^
- Followed by one or more non-path characters
[^\/\\:?*]+
- Ending with
.tsx
, etc\.(tsx?|jsx?|mdx?)$
To match flat-folders
convention, this is the regex: [\/\\]_index\.(tsx?|jsx?|mdx?)$
- Path ends with
/_index.tsx
- So it should ignore colocated files like
users/component.tsx
Put them together and you get: ((^[^\/\\:?*]+)|([\/\\]_index))\.(tsx?|jsx?|mdx?)$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using a regex would using isIndexRoute
be sufficient for non flat files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, considering the new core feature is a subset of the remix-flat-routes
package. You don't need to handle quite as many cases.
But if you end up expanding the naming conventions, you'll most likely need to use regex at some point.
Signed-off-by: Logan McAnsh <[email protected]>
Signed-off-by: Logan McAnsh <[email protected]>
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
closes #5028
before:
after:
Testing Strategy: