Skip to content

chore: remove spec files from runtime package #236

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

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

PhearZero
Copy link
Contributor

Overview

Looking at the package, everything seemed like it should not include the spec files. After some investigation I found some strange behavior with the components package being included in the build 🤔.

Summary

Package does not respect the tsconfig.build.json when @tutorialkit/components-react is included in the --filter.

Fix

The quick and dirty fix is to just exclude the @tutorial/components-react from the filter and have a two step build. It may need further triage with pnpm/typescript. Happy to dig deeper!

Copy link

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

@AriPerkkio
Copy link
Member

Thanks for looking into this!

Package does not respect the tsconfig.build.json when @tutorialkit/components-react is included in the --filter.

Oh wow, this is the case indeed. As soon as @tutorialkit/components-react is dropped from build script, test files are excluded. This seems to happen even in #155 where the package was moved to packages/react, so it seems this is not caused by nested directories.

It would be great to figure out why exactly this is happening. 🤔

@PhearZero
Copy link
Contributor Author

Anytime! Thank you all for the project ❤️. I was shocked as well!

It would be great to figure out why exactly this is happening. 🤔

If someone doesn't get to it, I'll try to dig in this evening (it's going to be non-obvious). My first instinct is to dig into pnpm v8.15.6. I'll report back here with any findings 🕵️

@Nemikolh
Copy link
Member

Hey @PhearZero, thanks for investigating! This is indeed really odd.

I wonder if this is a bug with the fact that the package is marked as composite but project reference default to the tsconfig.json. I think if we modify the { "path": "..." } references to point to the tsconfig.build.json it will work?

@PhearZero PhearZero force-pushed the chore/remove-spec-files branch from ffa2890 to 4378412 Compare August 12, 2024 23:34
@PhearZero
Copy link
Contributor Author

Thanks for the hint @Nemikolh ❤️, I was able to update the references for the runtime to point to the tsconfig.build.json and that seems to have resolved the build!

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 good! Do you think you could update the reference everywhere? There's also the tsconfig.json in the same folder as the file you modified and the one in astro.

Those aren't used by the build script but they're used by VSCode and so I think it might also get those files to be transpiled? In any case, it's probably less error prone to always reference the correct config everywhere.

@PhearZero PhearZero force-pushed the chore/remove-spec-files branch 2 times, most recently from b54ea08 to f1d18d8 Compare August 13, 2024 20:59
@PhearZero
Copy link
Contributor Author

@Nemikolh Makes total sense, I've updated the the references to point to the build configurations

@AriPerkkio
Copy link
Member

AriPerkkio commented Aug 16, 2024

@PhearZero the CI is failing on lint step. Could you run pnpm run lint --fix and commit the changes.

Or check the Allow edits and access to secrets by maintainers for this PR and I'll push the required changes.

@PhearZero PhearZero force-pushed the chore/remove-spec-files branch from f1d18d8 to 840c92f Compare August 16, 2024 13:15
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! But Github won't allow me to merge this as the branch is not up-to-date with main.

@PhearZero could you check this checkbox on from the right side of the page and I can proceed with this PR?

image

@PhearZero
Copy link
Contributor Author

@AriPerkkio it looks like edits by maintainer is not supported in a fork from another organization

I was able to use the Update branch button to merge in main

@AriPerkkio AriPerkkio merged commit a580bb5 into stackblitz:main Aug 20, 2024
9 checks passed
@AriPerkkio
Copy link
Member

Oh right, you forked TutorialKit repo to another Github organization instead of personal account. That makes sense.

Thanks for the fix!

@PhearZero PhearZero deleted the chore/remove-spec-files branch August 20, 2024 12:09
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.

@tutorialkit/runtime release includes test files
3 participants