Skip to content
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

chore(package size): implement findup-sync in @redwoodjs/project-config #8315

Merged
merged 13 commits into from
May 18, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented May 13, 2023

Note

I'm ok with adding a new function to @redwoodjs/project-config because it needs it. But I'm not intending for @redwoodjs/project-config to become a package where utilities go.

findup-sync isn't huge, but it's much bigger than it needs to be for our purposes. Let's get rid of it for our own custom implementation that's less generic but get the the job done, and continue chipping away at @redwoodjs/project-config's size till it's only as big as it needs to be.

findup-sync's package size stats: (courtesy of https://packagephobia.com/result?p=findup-sync)

publish size install size
7.62 kB 374 kB

This PR adds a new function to @redwoodjs/project-config, findUp, which looks up directories for a file:

import fs from 'fs'
import path from 'path'
/**
* Find a file by walking up parent directories.
*/
export function findUp(
file: string,
startingDirectory: string = process.cwd()
): string | null {
const possibleFilepath = path.join(startingDirectory, file)
if (fs.existsSync(possibleFilepath)) {
return possibleFilepath
}
const parentDirectory = path.dirname(startingDirectory)
// If we've reached the root directory, there's no file to be found.
if (parentDirectory === startingDirectory) {
return null
}
return findUp(file, parentDirectory)
}

It's as simple as it needs to be. It doesn't need to be more generic (i.e. take a function); doesn't need to be async, etc. It just needs to find redwood.toml.

This PR includes a lot of changes because I removed findup-sync everywhere I could. One of those places is the codemods package. Since findUp lives in @redwoodjs/project-config, I had to add it as a dependency, but this is a good thing because now I can get rid of the needless duplication in the codemods package.

@jtoar jtoar added the release:chore This PR is a chore (means nothing for users) label May 13, 2023
@thedavidprice
Copy link
Contributor

@jtoar aside from package size (which I agree is critical for our needs), how else are you evaluating look-it-up as a stable package?

@jtoar
Copy link
Contributor Author

jtoar commented May 13, 2023

@thedavidprice

  • it passes our tests
  • reading the source, looking at its package.json; general stats about the repo like how many issues, time to close, commits, etc. But most of that is ancillary to just looking at what it actually does

@jtoar jtoar marked this pull request as draft May 15, 2023 17:11
@jtoar jtoar changed the title chore(package size): swap findup-sync for look-it-up chore(package size): implement findup-sync in @redwoodjs/project-config May 18, 2023
@jtoar jtoar marked this pull request as ready for review May 18, 2023 22:47
Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

Woah, you tidied things up all over the place. Thank you for choosing this implementation. Definitely feels worth the effort!

@jtoar jtoar merged commit 93838d3 into main May 18, 2023
@jtoar jtoar deleted the ds-project-config/swap-out-findup-sync branch May 18, 2023 23:37
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone May 18, 2023
dac09 added a commit to dac09/redwood that referenced this pull request May 19, 2023
…te-default

* 'main' of github.com:redwoodjs/redwood: (23 commits)
  chore(deps): update dependency @clerk/clerk-react to v4.16.2 (redwoodjs#8362)
  chore(package size): implement `findup-sync` in `@redwoodjs/project-config` (redwoodjs#8315)
  Refactor GraphQL Server and CreateYoga to Support "api serve" with Fastify Server (redwoodjs#8339)
  chore(deps): update dependency octokit to v2.0.15 (redwoodjs#8360)
  fix(coherence): correct doc links, add commas to template (redwoodjs#8351)
  Parse as int, fix jsdoc (redwoodjs#8357)
  Update forms.md (redwoodjs#8352)
  chore: update yarn.lock
  chore(release): update release command for minors
  chore(deps): update dependency rimraf to v5.0.1 (redwoodjs#8350)
  chore(deps): update dependency glob to v10.2.5 (redwoodjs#8349)
  feat(coherence deploy): add setup deploy coherence (redwoodjs#8234)
  fix(deps): update dependency listr2 to v6.6.0 (redwoodjs#8347)
  fix(deps): update dependency react-router-dom to v6.11.2 (redwoodjs#8345)
  fix(deps): update prisma monorepo to v4.14.1 (redwoodjs#8346)
  fix(deps): update dependency webpack to v5.83.1 (redwoodjs#8348)
  chore(deps): update dependency dependency-cruiser to v13 (redwoodjs#8322)
  chore(deps): update dependency @clerk/clerk-react to v4.16.1 (redwoodjs#8324)
  chore(deps): update dependency @clerk/types to v3.38.0 (redwoodjs#8325)
  chore(deps): update dependency nx to v16.2.1 (redwoodjs#8343)
  ...
jtoar added a commit that referenced this pull request May 19, 2023
…onfig` (#8315)

* chore: swap out findup-sync for lighter alt

* cli

* rest

* update readme

* update dep graphs

* fix lint

* swap external package for our own

* follow-up fixes

* fix eslint fn

* update dep cruiser to 13

* lint fix
@jtoar jtoar modified the milestones: next-release, next-release-patch, v5.2.4 Jun 2, 2023
jtoar added a commit that referenced this pull request Jun 2, 2023
…onfig` (#8315)

* chore: swap out findup-sync for lighter alt

* cli

* rest

* update readme

* update dep graphs

* fix lint

* swap external package for our own

* follow-up fixes

* fix eslint fn

* update dep cruiser to 13

* lint fix
@jtoar jtoar modified the milestones: v5.2.4, next-release, v5.3.0 Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants