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

feat: sync files from WebContainer to editor #334

Merged
merged 12 commits into from
Sep 18, 2024
Merged

Conversation

Nemikolh
Copy link
Member

This PR adds a new property filesystem to the metadata of a Tutorial / Chapter / Lesson:

filesystem:
  syncChanges: true # default to false

When syncChanges is set to true, if files are modified on WebContainer via a terminal or by a process spawned manually via the webcontainer export on tutorialkit:core, the changes will be reflected in the editor.

Closes #208

This is opt-in because it can have a performance impact on the UI if lots of files are being modified / generated.

In the future we will likely accept boolean | string for syncChanges to be able to narrow down the watched files and improve performance.

Copy link

stackblitz bot commented Sep 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

cloudflare-workers-and-pages bot commented Sep 16, 2024

Deploying tutorialkit-demo-page with  Cloudflare Pages  Cloudflare Pages

Latest commit: c716577
Status: ✅  Deploy successful!
Preview URL: https://987bfb79.tutorialkit-demo-page.pages.dev
Branch Preview URL: https://joan-sync-files-from-fs.tutorialkit-demo-page.pages.dev

View logs

@Nemikolh Nemikolh marked this pull request as ready for review September 17, 2024 08:21
Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Couple of comments/questions before going through the code. Code review coming tomorrow, but overall it looks good.

  • Does this enable users to create files from terminal/commands so that they appear in file editor? If not, maybe this doesn't close Update _files content with scrips command #208?
  • We should keep feat: add template.visibleFiles option #165 in mind during this PR. I think that could be filesystem.visible: string | string[].
  • Naming of filesystem.syncChanges. As this could in future also allow globs for watched files, should we rename this to filesystem.watch instead?

@Nemikolh
Copy link
Member Author

Thanks for the questions! 😃 Answering inline:

Does this enable users to create files from terminal/commands so that they appear in file editor?

It does not at the moment. I was thinking we would do this later. Maybe as part of the filesystem.visible work you started?

What do you think?

If not, maybe this doesn't close #208?

I think it does. Or at least based on what the first comment says:

I'm creating a NgRx (Angular state management library) and the first step is to add the library with the ng add @ngrx/store@latest. It adds the dependency in package.json and updates some files.

I added all these files in the _files folder as the student task is to run the script and sees how it affect the project.
However even if the command succeeds, the files are not updated.

I think we can consider it closed when we merge this PR.

We should keep #165 in mind during this PR. I think that could be filesystem.visible: string | string[].

Oh I really like that naming! 🌟

Naming of filesystem.syncChanges. As this could in future also allow globs for watched files, should we rename this to filesystem.watch instead?

Ah yeah this makes more sense, I'll rename it 👍

@AriPerkkio
Copy link
Member

Does this enable users to create files from terminal/commands so that they appear in file editor?

It does not at the moment. I was thinking we would do this later. Maybe as part of the filesystem.visible work you started?

What do you think?

Sounds good to me, let's leave it to the next PR.

Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice work!

But one last thing before merging this: It would be good to add test case for verifying that when filesystem.watch is false or the default value, changes from webcontainer are not reflected to store. As this can have an impact on performance, we should be careful not to always enable this feature by accident.

@Nemikolh
Copy link
Member Author

Oh very good point!!

Dang, I knew there was something missing with testing and this PR 😅 , I'll add it 👍

Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Test case looks good, thanks for adding it! 💯

@Nemikolh Nemikolh merged commit 5c1de69 into main Sep 18, 2024
11 checks passed
@Nemikolh Nemikolh deleted the joan/sync-files-from-fs branch September 18, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update _files content with scrips command
2 participants