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 new files from WebContainer to editor #394

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

isaacplmann
Copy link
Contributor

@isaacplmann isaacplmann commented Oct 25, 2024

I work at Nx and in order to use tutorialkit for our tutorials, we need the editor to show files that are created by terminal commands.

This PR shows a prototype that enables the editor to show new files in the webcontainer that are created in a specific set of paths. This is needed to use tutorialkit for Nx tutorials.

Updates the watch lesson property to take either a boolean or string[]. If passing a string[] it is interpreted as a list of globs.

I added e2e tests for the new functionality as well.

If watch is false or undefined, existing files are not synced and new files are not created.
If watch is true, existing files are synced, but new files are not created.
If watch is an array of glob strings, existing files are synced and new files whose paths match one of the globs listed in the watch array are created in the editor. Files that are deleted in one of the glob patterns are removed from the editor. New files that are not in those paths do not get added to the editor.

Copy link

stackblitz bot commented Oct 25, 2024

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

@AriPerkkio AriPerkkio requested a review from Nemikolh October 27, 2024 08:33
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.

In #334 it was discussed whether we should allow fs.watch: [<glob>, <glob>]. I think instead of adding new addNewFilesInPaths, we should instead make this a default functionality of fs.watch. And add pattern matching for fs.watch to avoid performance issues.

@Nemikolh has better understanding of fs.watch so let's wait for his comments before any changes.

@isaacplmann
Copy link
Contributor Author

That API would work for me. I'm happy to implement it once @Nemikolh has a chance to review.

@Nemikolh
Copy link
Member

Hey! Thanks a ton for the PR 😃

The proposal of having watch accepting [<glob>] to restrict what get reflected in the file tree, sounds really good! 🙌

In terms of performance concerns, we won't be able to provide that down to fs.watch just yet. It's something we want to add at some point though but it should not block this PR from landing.

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

Once this is implemented using watch, we can merge it ✨

packages/runtime/src/store/tutorial-runner.ts Outdated Show resolved Hide resolved
packages/runtime/src/store/tutorial-runner.ts Outdated Show resolved Hide resolved
@isaacplmann isaacplmann force-pushed the add-new-files-in-paths branch from b920c77 to 00f514a Compare October 29, 2024 19:01
@isaacplmann
Copy link
Contributor Author

PR and description updated to allow an array in the filesystem.watch property

@isaacplmann
Copy link
Contributor Author

  • Now accounting for rename file deletion
  • Folder creation logic moved into scheduleReadFor
  • Use picomatch/posix
  • e2e tests for everything

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

Fantastic work on this @isaacplmann ! 🤩

Love it 😍

e2e/src/components/ButtonDeleteFile.tsx Outdated Show resolved Hide resolved
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.

Yup the code looks good to me. I'll do some manual testing before next release. Probably tomorrow.

@AriPerkkio
Copy link
Member

AriPerkkio commented Oct 31, 2024

/pkg-pr-new

⚡️ Your npm packages are published.
@tutorialkit/react: npm i https://pkg.pr.new/@tutorialkit/react@e288f4e
@tutorialkit/astro: npm i https://pkg.pr.new/@tutorialkit/astro@e288f4e
@tutorialkit/runtime: npm i https://pkg.pr.new/@tutorialkit/runtime@e288f4e
@tutorialkit/theme: npm i https://pkg.pr.new/@tutorialkit/theme@e288f4e
@tutorialkit/types: npm i https://pkg.pr.new/@tutorialkit/types@e288f4e

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.

There's a bug in new file/folder creation. Empty folders are added/duplicated. Is this just related to the test case or an actual bug?

tk-fs-bug.mov

e2e/test/filesystem.test.ts Outdated Show resolved Hide resolved
e2e/test/filesystem.test.ts Outdated Show resolved Hide resolved
e2e/test/filesystem.test.ts Outdated Show resolved Hide resolved
packages/types/src/schemas/common.ts Show resolved Hide resolved
@isaacplmann
Copy link
Contributor Author

Updated documentation as well

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, thanks @isaacplmann! 💯

@AriPerkkio AriPerkkio merged commit 3beda90 into stackblitz:main Nov 5, 2024
11 checks passed
@AriPerkkio
Copy link
Member

Released in 1.2.0.

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.

3 participants