🦋 Changeset detected

Latest commit: 3f1f082

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Good call, I'll update docs. The lint action fail is happening in main so it's unrelated.

No problem with the docs PR making this change to be accurate!

BUT to check:

Astro will not process your script tags in some situations. In particular, adding type="module" or any attribute other than src to a <script> tag will cause Astro to treat the tag as if it had an is:inline directive. The same will be true when the script is written in a JSX expression.

• if we are changing stated behaviour that people relied on, does this mean that some people might have written their scripts with certain expectations that are no longer true, and their project now works differently? Should we maybe add this note about scripts written in JSX expressions changing in the upgrade guide entry for renderScript, given that tons of people haven't upgraded yet and may still be using this guide for a while? That would handle the breaking change at the time it was intended (upgrade to v5 and directRenderScript becoming the default)

This PR adds the ability to conditionally render processed scripts

• the docs changed don't seem to reference anything about a new option to allow conditional rendering? So, should this changeset should be more marked as a "fix that conditional rendering processed scripts wasn't working as intended" because there is not really some new feature for the docs to describe?

if we are changing stated behavior that people relied on, does this mean that some people might have written their scripts with certain expectations that are no longer true, and their project now works differently?

That's a great question! It's possible some folks are probably still relying on the old script behavior, and their sites will behave differently after upgrading if we keep this as a patch release. To make the transition smoother we could go with a minor release instead, then for the next Astro minor, we can make the "breaking" change to the script behavior, and add a note to the changelog so people are aware of the change.

Should we maybe add this note about scripts written in JSX expressions changing in the upgrade guide entry for renderScript, given that tons of people haven't upgraded yet and may still be using this guide for a while? That would handle the breaking change at the time it was intended (upgrade to v5 and directRenderScript becoming the default)

I think adding a note to the upgrade guide is a good idea! Was actually looking for a place to add it, so this is perfect.

the docs changed don't seem to reference anything about a new option to allow conditional rendering? So, should this changeset should be more marked as a "fix that conditional rendering processed scripts wasn't working as intended" because there is not really some new feature for the docs to describe?

That's a good point! This is technically a fix as since v5, it was intended that processed scripts could be rendered conditionally.
(If this was a new feature, it would be a breaking change and would require a major release if not under a flag, i.e Astro 6 😅)

I'll update the changeset to reflect that. Thanks for the feedback! 🙌

Hmm I didn't expect this to be possibly as big of a breaking change, but I think it might still be worth fixing this so we get a consistent conditional rendering behaviour like shown in the issue. I'd still consider this a bug fix, but of the potentially breaking kind. And if it turns out to be quite breaking we could consider reverting the behaviour and queue this for Astro 6. My bad for no thinking this through before 😅

Agreed! I think it's worth fixing this bug now, and we can mention this in the minor version blog post if we push this as a minor release. That way, those upgrading to next minor can be aware of the change and update their scripts accordingly, and current users are shielded (assuming most folks pin on minor version and that they read release notes 😅). I feel like this kinda breaks semver though, but I think it's the most happy path to fix this bug if we wanna push this before Astro 6.

And yeah, we can always revert if it becomes too big of a problem. Will lookout in support to gauge it.

I'm not sure how we can only limit this to the Astro minor. We have "@astrojs/compiler": "^2.10.3" which would allow minor releases of the compiler. Even if we use ~ and publish that, users who've not upgraded Astro could've also only upgraded @astrojs/compiler still and hit the issue. If we want to limit this as an Astro minor, we likely have to expose a new option from the compiler which we can enable in the Astro minor. I personally am also fine with releasing this PR as is though and check how this impacts people.

Good catch! I wrongly assumed that we pinned the compiler with ~. Agreed with merging this as is then!

I'd still consider this a bug fix, but of the potentially breaking kind.

Yes, I think that's fine (if a little sneaky? 😅 )

Like I suggested, I think making sure this breaking changes is reflected in the upgrade guide allows this to be considered a fix of a thing we intended to break, just didn't properly break for v5! We should use the changeset to be clear that this is fixing an issue with a previously breaking change to get it to work as intended, so you might actually see the breaking change NOW -- I think that's our story, and we're sticking to it! (And, avoiding the need to wait until v6)

Awesome, thanks for your reviews and suggestions!