Replies: 9 comments 11 replies
-
|
If I remember correctly the reason why |
Beta Was this translation helpful? Give feedback.
-
|
i made a couple of WIP branches here in case anyone gets to finish them off: switching from execa to tinyexec: this one doesn't build yet since jest seems to behind the times re ESM. so it can't load tinyexec it or any other ESM package it seems removing fs-extra from create-react-router: removing recursive-readdir from create-react-router: and some other findings:
|
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
I'll add on eslint, I was curious to try it out (port CRA config for legacy eslint to flat config) and got this: https://github.com/outslept/eslint-config-react-app-x This works locally with |
Beta Was this translation helpful? Give feedback.
-
also Note Consider leveraging pnpm's hoisting mechanism to eliminate duplicate dependencies between root and workspace packages. Dependencies declared in the root |
Beta Was this translation helpful? Give feedback.
-
|
And in addition to the note above I ran knip preconfigured like this (feel free to tweak the settings and try to run it as well, I'll be happy if you can find something else): Knip Configurationimport { readFileSync, existsSync, readdirSync } from 'node:fs';
import { join } from 'node:path';
function getWorkspaces() {
const workspaces = ['integration'];
try {
workspaces.push(...readdirSync('integration/helpers', { withFileTypes: true })
.filter(d => d.isDirectory())
.map(d => `integration/helpers/${d.name}`));
} catch {}
try {
workspaces.push(...readdirSync('packages', { withFileTypes: true })
.filter(d => d.isDirectory())
.map(d => `packages/${d.name}`));
} catch {}
try {
workspaces.push(...readdirSync('playground', { withFileTypes: true })
.filter(d => d.isDirectory())
.map(d => `playground/${d.name}`));
} catch {}
return workspaces;
}
function getTsupEntry(path) {
const cfg = join(path, 'tsup.config.ts');
if (!existsSync(cfg)) return [];
const match = readFileSync(cfg).toString().match(/const entry = \[(.*?)\];/s);
if (!match) return [];
return match[1]
.split(',')
.map(e => e.trim().replace(/['"]/g, ''))
.filter(Boolean)
.filter(e => !['index.ts', 'main.ts', 'cli.ts'].includes(e));
}
function getProjectPatterns(path) {
const base = ['**/*.{ts,tsx,js,jsx}', '!node_modules/**', '!dist/**', '!build/**'];
if (path === 'packages/react-router-dev') {
return [...base, '!config/defaults/**', '!__tests__/utils/**', '!__tests__/fixtures/**'];
}
if (path === 'packages/react-router') {
return [...base, '!node-main*.js', '!lib/dom/node-main.js', '!lib/server-runtime/.eslintrc.js', '!__tests__/utils/**'];
}
if (path === 'packages/create-react-router') {
return [...base, '!__tests__/msw-register.ts', '!__tests__/fixtures/**'];
}
return base;
}
const config = {
includeEntryExports: true,
ignoreUnresolved: [/^\.\/\+types\//,],
workspaces: {
'.': {
entry: ['scripts/*.{js,ts,mjs}'],
project: ['scripts/**/*', 'jest/**/*', '*.{js,ts,mjs}'],
ignore: ['scripts/utils.js']
}
}
};
for (const workspace of getWorkspaces()) {
const entries = getTsupEntry(workspace);
config.workspaces[workspace] = {
entry: entries.length ? entries : undefined,
project: getProjectPatterns(workspace),
includeEntryExports: true
};
}
export default config;I manually went through all entries and marked what can be removed/deleted with some notes at the end of the details. Unused files (13)Most of these can be ignored as they are config files, etc. The Unused exports (49)I'm not touching the types to avoid messing with what you want to expose in the public API. |
Beta Was this translation helpful? Give feedback.
-
|
i wonder if some of those entry points can be inferred since, iirc, knip checks what |
Beta Was this translation helpful? Give feedback.
-
|
Really nice finding! I've updated the config to reflect this |
Beta Was this translation helpful? Give feedback.
-
|
With these changes to |
Beta Was this translation helpful? Give feedback.
-
|
People may still want to use the server inside an existing node server. So if they use express, polka or some other server - they can support react router by middleware. With the suggested changes it's basically a generic middleware Maybe the node package could export that, though 🤔 |
Beta Was this translation helpful? Give feedback.
-
|
Maybe a better question is whether or not |
Beta Was this translation helpful? Give feedback.
-
|
I would move it to |
Beta Was this translation helpful? Give feedback.
-
|
wip branch main...outslept:react-router:refactor/knip-cleanup I also updated the knip config from above again, there are a couple more issues that need to be resolved before I can add this as a separate script to the repository |
Beta Was this translation helpful? Give feedback.
-
|
First and foremost, thanks so much @43081j for opening up this discussion and so many draft PRs! Really appreciate the effort to help cleanup and improve the wider ecosystem. As for PRs, let's slow down a bit on opening/merging in non-draft PRs (c.c. @MichaelDeBoey and @timdorr, though I do really appreciate y'all's efforts and enthusiasm 🙏) This isn't to say we don't want to make these changes, but with our new governance model we want the Steering Committee to first review and then indicate which changes we're open to community PRs via issues. I know many of these changes are small, but this is a great proposal to start practicing our new way of getting community engagement. Additionally, I just wanted to point out that @43081j also asked as much 😅
I definitely think we should move forward with some of these, but others (like the express oriented work) I'm a bit skeptical of. Either way, I'd love @brophdawg11 to be able to take a look (he's currently on vacation) and help triage what would be helpful to pick up. Happy of course also to hear from others who have already engaged on what they think is worth doing or should be avoided, I'm sure it'll make it easier for us to make decisions. Thanks all! And please bear with us as we are adopting this new governance model. I think long term it'll be really great for the health and velocity of this project, but in the meantime it'll take a little bit for us to ramp up to getting used to the process. |
Beta Was this translation helpful? Give feedback.
-
|
I've updated the original post to be clearer about following contribution guidelines I also mentioned this on the discord so am happy to discuss larger changes there if that helps |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @43081j! We might be a little delayed in reviewing this, just because we have a bit backlog and one of our members is out right now. It's been good to see the back and forth on the express issue in the Discord, thanks for following up on that! |
Beta Was this translation helpful? Give feedback.
-
|
I just added the |
Beta Was this translation helpful? Give feedback.
-
|
Hey folks - we finally got to discussing this on our latest steering committee meeting and we're open to some of these changes but also are not super interested in making changes just to move to the new shiny thing. I've marked the above list with markdown checkboxes indicating the things that are done - and a few we're not interested in with strikethroughs. If it has an empty checkbox, we might be open to it but please don't just open a PR. We don't want PRs coming in that we need to then spend our time doing perf analysis on (related). We want to see the data proving that the change is worth it before opening it up to a community PR. So if you're interested in taking on one of those open items, let's do the following:
Thanks everyone! |
Beta Was this translation helpful? Give feedback.
-
|
Hello @brophdawg11, @43081j I've started work on this task:
Install size:
Ansis is faster, but for terminal output it doesn't matter.
Chalk v4 (last changes 4 years ago) cannot be updated to actual v5, because v5 is pure ESM while the codebase still relies on CJS: Ansis supports both ESM and CJS. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks! I asked @markdalgleish to take a look at this when he has some time since he's most familiar with the current CLI |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Remix is one of the few orgs we haven't interacted with much in the e18e community. I think it'd be nice to increase the collaboration here if you're interested. You can read about what the community is doing on e18e.dev.
this is just my list of findings. if any seem like something we should do here, please comment as I'm sure there will be community members willing to tackle them in PRs.
similarly, if any don't make sense, I'm happy to remove them from the list or note that people shouldn't contribute the change.
submitting prs
If anyone wants to pick any of this stuff up, please do follow the contribution guide/rules. In particular create issues for larger changes especially, and start with draft PRs if you already made the change
repo-wide tasks
Switch to ESLint flat config (eslint.config.js)rimraftopremove(orfs.rmif programmatic usage)rimraf#13779create-react-router
General changes
enginesconstraint to 20.x? since the other monorepo packages seem to already have done thisDependency changes
fs-extrawith nativefs.cp(...)fs-extra#13736execawith tinyexecchalkwith ansis (not picocolors because we use hex colours here)tar-fswithtar(latesttar-fsis much larger, but we haven't upgraded to it here yet. both seem to perform well enough for the usage here)recursive-readdirwith nativereaddir(path, {recursive: true})recursive-readdir#13735Refactors
react-router-dev
Dependency changes
fs-extrawith native FSfs-extra#13736lodashisEqualwithnode:util.isStrictDeepEqualcloneDeepwithstructuredCloneomitis only used once, so maybe we can just use spread or remove the keys manuallypickcould possibly be inlined as a util, or justfiltertheObject.entrieskebabCasewithsculelodash#14180react-router-fs-routes
Dependency changes
minimatchwithzeptomatch(low priority change of course)Refactors
replaceAllinstead of asplit/joinin normalizeSlashes (should be faster, but if not, areplacewith a global regex should be)replaceAllfor slash normalisation #13738ignoreFileRegexafter the various error checks to avoid it being wasted/garbage collectedreact-router-serve
Dependency changes
possibly switch to a lighter web server than express since it doesn't need to be pluggable or anything for this particular usereact-router-express
Refactors
Rework to support non-express serversexisting work
cc @MichaelDeBoey since you were already doing some of this
Beta Was this translation helpful? Give feedback.
All reactions