Remove existing mirrored cells before generating new #5820

Tobbe · 2022-06-24T13:40:49Z

Splits the generator file watching into two. One for the API side and one for the Web side.
The Web side watcher now also watches for directory changes. It was the only way to catch directories that were being renamed. When a Cell directory is unlinked we now also remove the corresponding mirror directory.

netlify · 2022-06-24T13:40:55Z

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	`5fec6ff`
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62c053007832140007046671

dac09 · 2022-06-24T15:50:14Z

My only issue with this stuff is that we're increasing file system operations on every change. This really does add up over time. Can we not remove the files specifically when its renamed/removed?

Tobbe · 2022-06-24T15:59:02Z

@dac09 I don't think it's too bad, because this code is only run when you manually run yarn rw g types or when you run some other command that in turns run the type generator.

To specifically remove the affected files when something is renamed/removed we'd need to run a watcher to detect the renaming/removal, right? Or do we already have a watcher on the cells I can leverage?

dac09 · 2022-06-24T16:06:07Z

We already have watchers for each type - that’s how we know to generate mirror cells I think!

Won’t it run when running the generate subprocess in rw dev?

Tobbe · 2022-06-24T16:27:28Z

No, we explicitly run generateTypes or whatever it's called as a separate step in the cell generator. No watchers for that stuff

dac09 · 2022-06-24T17:06:11Z

I believe its the same function as we use in the watcher. If you check packages/internal/package.json - the rw-gen and rw-gen-watch commands reference the same generate function - so it will be triggered on any file changes

See packages/internal/src/generate/watch.ts for where we filter Cell files, and we use the “all” event, but we could be smarter about this.

Tobbe · 2022-06-24T17:17:22Z

Ohh, so we do both! TMYK. Thanks Danny. I'll see what I can do then.

dac09 · 2022-06-24T17:25:10Z

✌️

Tobbe · 2022-07-03T08:44:16Z

@dac09 This is ready for a new review. Thanks :)

dac09 · 2022-07-04T15:51:16Z

packages/internal/src/files.ts

-  // A Cell must be a directory named module.
-  if (!dir.endsWith(name)) {
+  // A cell must end with "Cell.{jsx,js,tsx}".
+  if (!name.endsWith('Cell')) {


Why this change?

I'm wondering if this has a downstream impact elsewhere?

We used to have the name check out in the watcher function, but I thought it was such a fundamental check that I wanted it to be part of the isCellFile check itself. So that we always check this when checking if a file is a Cell.

The reason I removed the "directory named module"-check is because I think it's wrong. In one of my projects I have a "child cell" (for a waterfall kind of setup), and I chose to put that Cell in the same directory as the parent Cell. Works great.

This also aligns with the comment in RWCell.ts in the structure package:

/** * A "Cell" is a component that ends in `Cell.{js, jsx, tsx}`, has no * default export AND exports `QUERY` **/

And it aligns with our public facing docs

Redwood looks for all files ending in "Cell" (so if you want your component to be a Cell, its filename does have to end in "Cell"), but if the file 1) doesn't export a const named QUERY and 2) has a default export, then it'll be skipped.
https://redwoodjs.com/docs/cells#how-does-redwood-know-a-cell-is-a-cell

As far as downstream impact goes the isCellFile function is used (by extension) in our type generator and directly in the watcher. So I think we're good on that front.

Looking at all this again I think maybe we should also remove the check for a Success export below to further align this implementation with the other code and docs

Would the generated mirror types be correct for a non-directory-import Cell? If so, happy with it and makes sense.

Also agreed about the fact that a cell shouldn't need to be in a directory named module.

The project I'm referring to is actually a JS project. So I only know that the basic Cell functionality works. Don't know about the types. I'll make sure to test it manually.

packages/internal/src/generate/watch.ts

dac09 · 2022-07-04T16:22:20Z

Bit of feedback here as well @Tobbe - https://s.tape.sh/LiBfply6

Not working as expected on Gitpod/linux

dac09

Left a few comments, and test on gitpod didn't quite work!

Tobbe

@dac09 replied to your code comments. Thanks for your review as always. Will watch your tape next.

Tobbe · 2022-07-04T18:41:49Z

packages/internal/src/files.ts

-  // A Cell must be a directory named module.
-  if (!dir.endsWith(name)) {
+  // A cell must end with "Cell.{jsx,js,tsx}".
+  if (!name.endsWith('Cell')) {


We used to have the name check out in the watcher function, but I thought it was such a fundamental check that I wanted it to be part of the isCellFile check itself. So that we always check this when checking if a file is a Cell.

The reason I removed the "directory named module"-check is because I think it's wrong. In one of my projects I have a "child cell" (for a waterfall kind of setup), and I chose to put that Cell in the same directory as the parent Cell. Works great.

This also aligns with the comment in RWCell.ts in the structure package:

/** * A "Cell" is a component that ends in `Cell.{js, jsx, tsx}`, has no * default export AND exports `QUERY` **/

And it aligns with our public facing docs

Redwood looks for all files ending in "Cell" (so if you want your component to be a Cell, its filename does have to end in "Cell"), but if the file 1) doesn't export a const named QUERY and 2) has a default export, then it'll be skipped.
https://redwoodjs.com/docs/cells#how-does-redwood-know-a-cell-is-a-cell

As far as downstream impact goes the isCellFile function is used (by extension) in our type generator and directly in the watcher. So I think we're good on that front.

Looking at all this again I think maybe we should also remove the check for a Success export below to further align this implementation with the other code and docs

packages/internal/src/generate/watch.ts

Tobbe · 2022-07-04T19:26:11Z

Thanks for the tape @dac09

I've noticed that it's a bit flaky on my Mac as well unfortunately, especially with detecting deleted folders as you noticed. I will play around with it a bit in GitPod and see if the event names etc are the same as on Mac.

Not sure here, but for deleted folders, if we don't detect that, potentially we'll have "orphaned" mirror files in .redwood/, right? But as long as those don't have a an actual Cell in /web/src/... to merge with they won't actually do anything, will they?

I was thinking of adding a "garbage collection" step in watcher.on('ready'), so on startup it will first always delete all mirror files, and then generate new ones. What do you think about that?

Also, how do you feel about setting the bar at "better than it was before" instead of "perfect"? I'd really like to aim for "perfect" but I just don't know how feasible that is.

EDIT: Just realized another thing. If the user does some changes, like renaming or deleting a folder, while the dev server isn't running we need the GC step to get .redwood/ back in a good state when the user eventually does start the dev server back up. Also need to make sure manually running yarn rw g types does the GC

dac09 · 2022-07-04T19:57:37Z

packages/internal/src/generate/watch.ts

-  unlink: 'Deleted',
-  change: 'Modified',
-}
+const dirWatcher = chokidar.watch('src', {


Was there a reason to separate these watchers? I'm not 100% sure, but I think spinning up more watchers is discouraged, especially on systems with a big FS penalty e.g. Windows, and certain scenarios with docker.

Yes. It's a trade-off I guess. This dirWatcher is going to be watching a lot more files because of the less restrictive pattern. To limit that, I split it up so that I could keep the more restrictive pattern for the API side. Because on that side we don't have to watch for folder deletions.

But if you think it's still better to just watch everything across both the api side and the web side with a single watcher I can easily switch to that instead

I can't speak with full confidence which is better really! Let's leave it as is, but worth doing a check on Windows maybe, but its also super hard to quantify the impact of something like this.

dac09 · 2022-07-04T20:00:06Z

I was thinking of adding a "garbage collection" step in watcher.on('ready'), so on startup it will first always delete all mirror files, and then generate new ones. What do you think about that?

Yeah that will likely solve a bunch of cases, especially if we do a fresh generate right after.

Also, how do you feel about setting the bar at "better than it was before" instead of "perfect"? I'd really like to aim for "perfect" but I just don't know how feasible that is.

Yeah totally ok 👌, was pointing out incase you hadn't noticed, we don't need to solve all the edge cases now.

Stale

dac09 · 2022-12-13T07:30:55Z

Found this again @Tobbe - lets pick this up again?

Tobbe · 2022-12-13T08:01:00Z

Would be really great to get a solution in place for this for sure. But for now I want to focus on auth 🙂

Tobbe · 2023-12-17T11:17:45Z

I'm not going to have time to prioritize this anytime soon. If anyone else wants to pick this one back up, please feel free to do so 🙂

redwoodjs-bot bot assigned Tobbe Jun 24, 2022

Tobbe added the release:fix This PR is a fix label Jun 24, 2022

Watch for folder deletion and delete corresponding mirror folders

5fec6ff

Tobbe force-pushed the tobbe-typegen-rm-first branch from a5fad0d to 5fec6ff Compare July 2, 2022 14:15

Tobbe marked this pull request as ready for review July 2, 2022 20:31

dac09 reviewed Jul 4, 2022

View reviewed changes

packages/internal/src/generate/watch.ts Show resolved Hide resolved

dac09 reviewed Jul 4, 2022

View reviewed changes

packages/internal/src/generate/watch.ts Show resolved Hide resolved

dac09 previously requested changes Jul 4, 2022

View reviewed changes

Tobbe commented Jul 4, 2022

View reviewed changes

dac09 reviewed Jul 4, 2022

View reviewed changes

Tobbe mentioned this pull request Jul 5, 2022

[Bug?]: Code navigation to a cell implementation not possible (ends up in type definition) #5862

Closed

1 task

Tobbe mentioned this pull request Mar 3, 2023

No code completion for cell component #4303

Open

Tobbe closed this Dec 17, 2023

Tobbe added the help wanted label Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove existing mirrored cells before generating new #5820

Remove existing mirrored cells before generating new #5820

Tobbe commented Jun 24, 2022 •

edited

Loading

netlify bot commented Jun 24, 2022 •

edited

Loading

dac09 commented Jun 24, 2022 •

edited

Loading

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jul 3, 2022

dac09 Jul 4, 2022

Tobbe Jul 4, 2022

dac09 Jul 4, 2022 •

edited

Loading

Tobbe Jul 4, 2022

dac09 commented Jul 4, 2022

dac09 left a comment

Tobbe left a comment

Tobbe Jul 4, 2022

Tobbe commented Jul 4, 2022 •

edited

Loading

dac09 Jul 4, 2022

Tobbe Jul 4, 2022 •

edited

Loading

dac09 Jul 4, 2022

dac09 commented Jul 4, 2022

dac09 commented Dec 13, 2022

Tobbe commented Dec 13, 2022

Tobbe commented Dec 17, 2023

Remove existing mirrored cells before generating new #5820

Remove existing mirrored cells before generating new #5820

Conversation

Tobbe commented Jun 24, 2022 • edited Loading

netlify bot commented Jun 24, 2022 • edited Loading

✅ Deploy Preview for redwoodjs-docs canceled.

dac09 commented Jun 24, 2022 • edited Loading

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jun 24, 2022

dac09 commented Jun 24, 2022

Tobbe commented Jul 3, 2022

dac09 Jul 4, 2022

Choose a reason for hiding this comment

Tobbe Jul 4, 2022

Choose a reason for hiding this comment

dac09 Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Tobbe Jul 4, 2022

Choose a reason for hiding this comment

dac09 commented Jul 4, 2022

dac09 left a comment

Choose a reason for hiding this comment

Tobbe left a comment

Choose a reason for hiding this comment

Tobbe Jul 4, 2022

Choose a reason for hiding this comment

Tobbe commented Jul 4, 2022 • edited Loading

dac09 Jul 4, 2022

Choose a reason for hiding this comment

Tobbe Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

dac09 Jul 4, 2022

Choose a reason for hiding this comment

dac09 commented Jul 4, 2022

dac09 commented Dec 13, 2022

Tobbe commented Dec 13, 2022

Tobbe commented Dec 17, 2023

Tobbe commented Jun 24, 2022 •

edited

Loading

netlify bot commented Jun 24, 2022 •

edited

Loading

dac09 commented Jun 24, 2022 •

edited

Loading

dac09 Jul 4, 2022 •

edited

Loading

Tobbe commented Jul 4, 2022 •

edited

Loading

Tobbe Jul 4, 2022 •

edited

Loading