search

stackblitz / tutorialkit

TutorialKit by StackBlitz - Create interactive tutorials powered by the WebContainer API
https://tutorialkit.dev
MIT License
232 stars 22 forks source link

fix(astro): resolve Astro's private APIs properly #238

Closed AriPerkkio closed 3 weeks ago

AriPerkkio commented 1 month ago

The usage of private APIs was added in https://github.com/stackblitz/tutorialkit/pull/4 (👋 @d3lm).

stackblitz[bot] commented 1 month ago

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

AriPerkkio commented 1 month ago

we'll be able to get rid of the type error

This is not just type error. In monorepo setups this import actually fails and TutorialKit fails to start. Here's example from https://github.com/AriPerkkio/tutorialkit/tree/test/ui-tests/test/ui which doesn't work without this fix.

d3lm commented 1 month ago

Yes, I agree with @Nemikolh and I'd like to keep it as is. The whole point to get this in upstream is to avoid copying code from Astro so we can fully rely on upstream for this. I think we can give them the feedback that it works well for us so it can be exposed properly and we don't need to import from node_modules directly. But I am not a fan of copying it again cause that's what we wanted to avoid in the first place.

AriPerkkio commented 1 month ago

Sure, that would be ideal case. It's also breaking change as we would need to set new minimum version of astro but we can do breaking change release at this point easily.

If you have any ideas how to fix #235 and https://github.com/stackblitz/tutorialkit/pull/241 without this change, feel free to help! Otherwise this will block those two, I think.

Nemikolh commented 1 month ago

@AriPerkkio For #235, I think we can temporarily use a config option. We should definitely provide feedback that this has been working fine for us.

For #241, why is this an issue there? Doesn't the dependency also get installed by pnpm? (I only had a quick look so sorry if it's something obvious 🙈)

AriPerkkio commented 1 month ago

Removed copied Astro's swap-functions.js and added Vite plugin that resolves it properly. I tried to add module resolving logic directly to Layout.astro but it just didn't work. It's best to do on Vite's side.

Tested with https://github.com/stackblitz/tutorialkit/issues/235 and https://github.com/stackblitz/tutorialkit/pull/241. Checked dev mode and build dist contents.

Using private APIs like this relies on Astro's npm package's file structure and can break at any time. There's reason why package.json's exports are required for imports. In my opinnion it would be best to simply copy-paste this from Astro if it's wanted to be used. And once they include it in their public API (exports), we can start requiring that version as peer dep.

d3lm commented 1 month ago

We should definitely provide feedback that this has been working fine for us.

@Nemikolh @AriPerkkio I can provide that feedback to them and ask them to expose it properly for us.

d3lm commented 1 month ago

Ok PR is on the way https://github.com/withastro/astro/pull/11708

Nemikolh commented 3 weeks ago

@d3lm, @AriPerkkio do we want to wait for that PR to land on Astro's side then? Or are we all happy to move forward with this PR?

AriPerkkio commented 3 weeks ago

Let's close this and wait for Astro's PR instead. I can add work-around into the #241 for now, and post same instructions in #235.