feat: add `template.visibleFiles` option #165

AriPerkkio · 2024-07-24T08:25:24Z

Adds template.visibleFiles option that can be used to toggle files from template visible in lessons
Example usage: feat: use template.visibleFiles to reduce repetition AriPerkkio/tutorial-vite-plugin#15

stackblitz · 2024-07-24T08:25:27Z

Run & review this pull request in StackBlitz Codeflow.

AriPerkkio · 2024-07-24T15:07:09Z

Running into #167 here too. When "Solve" is clicked, files from template are shown as empty. Only files in _solution are shown correctly.

~~Need to fix #167 first.~~ ✅

AriPerkkio · 2024-07-30T07:07:40Z

Tested manually with AriPerkkio/tutorial-vite-plugin#15 - the amount of removed duplicate files is a lot!

As this changes quite a lot how editor's files are resolved, I think we should wait for @Nemikolh or @SamVerschueren to review this next week.

AriPerkkio · 2024-07-30T07:11:32Z

docs/tutorialkit.dev/src/content/docs/guides/creating-content.mdx

+<Tabs syncKey="file-visibilty">
+  <TabItem label="Initially">
+
+```markdown ins=/.{24}├── (first.js)/ ins=/└── (second.js)/ ins=/third.js/


This patterns might look a bit weird. It's matching the words on specific rows. I don't think expressive-code supports syntax like ins={24, "first.js"} to match first.js on line 24 only.

Nemikolh

Looks good! I have a couple of comments but my biggest point is that I would want us to have visibleFiles entries match's git's .gitignore format.

On a TutorialKit project, I expect most of our users to use git to version their tutorial and so I think it's reasonable to expect from them to be more familiar with the .gitignore format than something else. I think it's similar to globs but a comment in this PR make me think that maybe it doesn't work exactly the same.

packages/astro/src/default/utils/content.ts

packages/types/src/schemas/common.ts

Nemikolh · 2024-08-05T09:24:57Z

packages/astro/src/default/utils/content.ts

@@ -347,6 +366,15 @@ async function getFilesRefList(pathToFolder: string): Promise<FilesRefList> {
  return [filesRef, filePaths];
 }

+function formatTemplateFile(filename: string) {
+  // compare files without leading "/" so that patterns like ["src/index.js"] match "/src/index.js"


I'm not super familiar with micromatch, but this comment and the logic after make me think that maybe this is not the library that we want?

I think we should have visibleFiles entries follow a similar format to git's gitignore entries. That would lower the learning curve of TutorialKit a lot and not necessitate as much trial and error for users.

In particular if you say:

template: visibleFiles: - /foobar

It should not match /test/foobar but match a /foobar file in the template.

If you mean to match every file named foobar then you can only say foobar or **/foobar.

I picked micromatch since it's already a transitive dependency from fast-glob.

.gitignore doesn't require prefixing all files with /? Meaning that:

$ touch new-file.ts $ git status Untracked files: (use "git add <file>..." to include in what will be committed) new-file.ts $ echo "new-file.ts" >> .gitignore $ git status Changes not staged for commit: modified: .gitignore

What formatTemplateFile here does, is that it allows you to define visibleFiles: ["src/main.js"] instead of ["/src/main.js"]. Do we want to revert this / handling?

Though I guess metadata's focus option does require / prefixes for all files.

@Nemikolh here's small playground to check how it works:
https://stackblitz.com/~/edit/stackblitz-starters-efozpk?file=index.mjs
node index.mjs to run.

I modified the example a bit to showcase what I mean: https://stackblitz.com/~/edit/stackblitz-starters-bq7pxo.

So basically when we have:

[ '/foobar/a.js', '/a.js', ]

If you have a .gitignore that says:

a.js

then it will ignore both files. But if it says /a.js, it will only ignore the second one.

With micromatch only, it doesn't work. We might make it work with a bit more work. Like if the input in the array does not start with / we can add **/ to it and it might produce the same behaviour.

Oh I see, thanks for the example! Does it work correctly if basename: true is used?

- const isMatch = micromatch.isMatch(file, [pattern]); + const isMatch = micromatch.isMatch(file, [pattern], { basename: true });

{ file: '/foobar/a.js', pattern: 'a.js', isMatch: true } { file: '/a.js', pattern: 'a.js', isMatch: true }

Oh looks like this is working! This might be good enough?

cc @SamVerschueren

Now using basename: true.

packages/runtime/src/store/index.ts

Nemikolh

Noticed a really important detail that regardless of the behaviour of the matching should be addressed.

Nemikolh · 2024-08-28T08:41:57Z

packages/runtime/src/store/index.ts


-        this._editorStore.setDocuments(files);
+        const editorFiles = { ...this._visibleTemplateFiles, ...this._lessonFiles };
+        this._editorStore.setDocuments(editorFiles);


I just noticed that the files from the template are not read-only.

Given that they aren't part of the lesson but are here to provide information on the project, I really feel like they should be read only. Otherwise it feels like we're moving too far away from the original goal of having tutorials be this "safe" environment where learners can focus on the learning.

I don't think we have options to make any file readonly at the moment. Maybe we need such feature, especially once adding new files via terminal becomes possible?

Currently it's intentional that files are modifiable. This is simply to reduce repetition - you don't have to copy-paste files in every single lesson just to make them visible.

I don't think we have options to make any file readonly at the moment. Maybe we need such feature, especially once adding new files via terminal becomes possible?

That is correct, until this PR we didn't have a need for them.

I'm not quite sure what we want to do about files being added from a terminal or more generally added in WebContainer by any mean, but I'm tempted to think that we'll want to design a solution that around a specific example.

Currently it's intentional that files are modifiable. This is simply to reduce repetition - you don't have to copy-paste files in every single lesson just to make them visible.

Then IMO it's the wrong abstraction. I never saw visibleFiles as intended to reduce repetition in _files. If we want that then we should maybe consider allowing .tk-config.json in _files (which I think already works but isn't documented) or have a different approach. This feels a bit hacky and against the original idea that templates are not part of the lesson (as in initial files to modify or the final solution).

Otherwise it feels like we're moving too far away from the original goal of having tutorials be this "safe" environment where learners can focus on the learning.

Yep, it sounds like we need to reconsider these changes. Maybe it's best to have only files from _files visible, as it currently is. Having files that aren't modifiable in file tree could make tutorials even more confusing.

Maybe this option shouldn't even be on template, it should be just file system based one. 🤔

Oh that's a good point actually! It's not really a template property but more like what we should show from the fs regardless of where it's coming from.

Hmm I agree maybe this is something we should discuss further, maybe we could start a thread on twitter about it and ping people that have requested the feature? Or maybe we can have the conversation in a GitHub issue.

AriPerkkio mentioned this pull request Jul 24, 2024

feat: use template.visibleFiles to reduce repetition AriPerkkio/tutorial-vite-plugin#15

Draft

AriPerkkio force-pushed the feat/template-visible-files branch 5 times, most recently from 3a11ee5 to 5368ef2 Compare July 24, 2024 13:30

AriPerkkio force-pushed the feat/template-visible-files branch 6 times, most recently from 0e4e9e4 to 4b0695d Compare July 30, 2024 06:53

AriPerkkio marked this pull request as ready for review July 30, 2024 07:06

AriPerkkio requested a review from Nemikolh July 30, 2024 07:07

AriPerkkio commented Jul 30, 2024

View reviewed changes

AriPerkkio mentioned this pull request Jul 30, 2024

templates/*/node_modules should be ignored #189

Closed

Nemikolh reviewed Aug 5, 2024

View reviewed changes

AriPerkkio added 4 commits August 13, 2024 07:58

feat: support object type in template schema

a62e777

feat: add template.visibleFiles option

57adec8

fix: code review

f498eb2

fix: code review

8f59cf9

AriPerkkio force-pushed the feat/template-visible-files branch from 1a64db3 to 8f59cf9 Compare August 13, 2024 05:07

test: update snapshots

5d050dc

AriPerkkio requested a review from SamVerschueren August 13, 2024 05:26

Nemikolh reviewed Aug 28, 2024

View reviewed changes

AriPerkkio marked this pull request as draft August 30, 2024 15:56

AriPerkkio mentioned this pull request Sep 17, 2024

feat: sync files from WebContainer to editor #334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `template.visibleFiles` option #165

feat: add `template.visibleFiles` option #165

AriPerkkio commented Jul 24, 2024 •

edited

Loading

stackblitz bot commented Jul 24, 2024

AriPerkkio commented Jul 24, 2024 •

edited

Loading

AriPerkkio commented Jul 30, 2024

AriPerkkio Jul 30, 2024

Nemikolh left a comment

Nemikolh Aug 5, 2024

AriPerkkio Aug 5, 2024 •

edited

Loading

AriPerkkio Aug 5, 2024

AriPerkkio Aug 5, 2024

Nemikolh Aug 5, 2024 •

edited

Loading

AriPerkkio Aug 6, 2024 •

edited

Loading

Nemikolh Aug 6, 2024

Nemikolh Aug 6, 2024

AriPerkkio Aug 6, 2024

Nemikolh left a comment

Nemikolh Aug 28, 2024

AriPerkkio Aug 28, 2024 •

edited

Loading

Nemikolh Aug 28, 2024

AriPerkkio Aug 28, 2024

AriPerkkio Aug 28, 2024

Nemikolh Aug 28, 2024

feat: add template.visibleFiles option #165

Are you sure you want to change the base?

feat: add template.visibleFiles option #165

Conversation

AriPerkkio commented Jul 24, 2024 • edited Loading

stackblitz bot commented Jul 24, 2024

AriPerkkio commented Jul 24, 2024 • edited Loading

AriPerkkio commented Jul 30, 2024

Choose a reason for hiding this comment

Nemikolh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriPerkkio Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemikolh Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

AriPerkkio Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemikolh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriPerkkio Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `template.visibleFiles` option #165

feat: add `template.visibleFiles` option #165

AriPerkkio commented Jul 24, 2024 •

edited

Loading

AriPerkkio commented Jul 24, 2024 •

edited

Loading

AriPerkkio Aug 5, 2024 •

edited

Loading

Nemikolh Aug 5, 2024 •

edited

Loading

AriPerkkio Aug 6, 2024 •

edited

Loading

AriPerkkio Aug 28, 2024 •

edited

Loading