-
Notifications
You must be signed in to change notification settings - Fork 102
feat: sync files from WebContainer to editor #334
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
Conversation
Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.
Deploying tutorialkit-demo-page with Cloudflare Pages Cloudflare Pages
c716577
@AriPerkkio
AriPerkkio
left a comment
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.
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 #208?
- We should keep #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 tofilesystem.watchinstead?
Nemikolh
commented
Sep 17, 2024
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 tofilesystem.watchinstead?
Ah yeah this makes more sense, I'll rename it 👍
docs/tutorialkit.dev/src/content/docs/reference/configuration.mdx
Outdated
Show resolved
Hide resolved
e2e/src/content/tutorial/tests/filesystem-sync/happy-path/_files/a/b/baz.txt
Show resolved
Hide resolved
AriPerkkio
commented
Sep 18, 2024
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.visiblework you started?What do you think?
Sounds good to me, let's leave it to the next PR.
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
@AriPerkkio
AriPerkkio
left a comment
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.
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
commented
Sep 18, 2024
Oh very good point!!
Dang, I knew there was something missing with testing and this PR 😅 , I'll add it 👍
@AriPerkkio
AriPerkkio
left a comment
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.
Test case looks good, thanks for adding it! 💯
This PR adds a new property
filesystemto the metadata of a Tutorial / Chapter / Lesson:When
syncChangesis set totrue, if files are modified on WebContainer via a terminal or by a process spawned manually via thewebcontainerexport ontutorialkit: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 | stringforsyncChangesto be able to narrow down the watched files and improve performance.