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

fix types export for astro 3 and typescript 5 #74

Merged

Conversation

jsve
Copy link
Contributor

@jsve jsve commented Oct 15, 2023

In addition to this it seems like type declarations also needs to be updated for the component to be imported without TS issues.

This PR replicates the component export process from @storyblok/astro

@jsve jsve changed the title replicate component export process from @storyblok/astro fix types export for astro 3 and typescript 5 Oct 15, 2023
@edvinasjurele
Copy link
Collaborator

Hey @jsve, thank you for your pull request 🙏

I see, we missed handling types correctly and I can reproduce it locally:

image

Also, found how storyblok/astro solved it https://github.com/storyblok/storyblok-astro/pull/454/files


This to address:

  1. DCO. Please check your commit signing
  2. Could you please fix the demo/src/pages/index.astro to have the following:
import type { RichTextType } from "storyblok-rich-text-astro-renderer";

instead of current:

import type { RichTextType } from "storyblok-rich-text-astro-renderer/RichTextRenderer.astro";

? And I am almost certainly sure the export type { RichTextType }; is redundant in the RichTextRenderer.astro from now on too, because RichTextRenderer.ts file covers the importing bits instead, and not .astro anymore.

Thanks.

@jsve jsve force-pushed the fix-types-for-astro-3-and-typescript-5 branch from a419c37 to 15f936c Compare October 16, 2023 08:46
@jsve
Copy link
Contributor Author

jsve commented Oct 16, 2023

Thanks for reviewing!

I did the changes and the commit from the web interface, hence the missing DCO. That also meant that I didn't get prettier and the other helpers. Sry!

I cleaned up the code now. Also had to modify the ts-config ref with jsx: preserve.

I agree that the export type is redundant in the RichTextRenderer.astro. It makes more sense to get all types from the actual types. I made the corresponding change in the readme as well.

@jsve jsve force-pushed the fix-types-for-astro-3-and-typescript-5 branch from 16c0081 to a062d65 Compare October 16, 2023 09:18
Copy link
Collaborator

@edvinasjurele edvinasjurele left a comment

Choose a reason for hiding this comment

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

👏 love it!

@edvinasjurele
Copy link
Collaborator

One more small bit though @jsve, we do not have squash and merge enabled (maybe it would be a good idea to consider), since we use semantic-release (not quite finished, some bits needed for CHANGELOG generating and CI) in the meantime, I would like you to squash your changes into single commit (fix: improve types export for astro 3 and typescript 5) please 🙏

@jsve jsve force-pushed the fix-types-for-astro-3-and-typescript-5 branch from a062d65 to 7be7425 Compare October 17, 2023 08:59
@jsve jsve force-pushed the fix-types-for-astro-3-and-typescript-5 branch from 7be7425 to 115c64f Compare October 17, 2023 09:02
@jsve
Copy link
Contributor Author

jsve commented Oct 17, 2023

no problem! Definitely sounds like you should enable squash and merge :)

I used your suggested commit message in the squashed commit. Shouldn't it be fix!: ... though? Aka breaking change? When we moved the export of the type, that potentially means that users need to modify their imports. Just a thought.

@edvinasjurele
Copy link
Collaborator

no problem! Definitely sounds like you should enable squash and merge :)

I used your suggested commit message in the squashed commit. Shouldn't it be fix!: ... though? Aka breaking change? When we moved the export of the type, that potentially means that users need to modify their imports. Just a thought.

You are right! Let's make a proper major release then. We can explain the breaking change, thus the commit may be either with ! mark or with that BREAKING CHANGE notice, like with explanation:

fix: improve types export for astro 3 and typescript 5

BREAKING CHANGE: The TS types imports has been changed.

@edvinasjurele edvinasjurele merged commit b132d10 into NordSecurity:main Oct 17, 2023
2 checks passed
@jsve
Copy link
Contributor Author

jsve commented Oct 17, 2023

Awesome! Thanks for helping me navigate your git-process :)

Do you have any plans for when the next release will be out? I'm guessing I'm not the only one who has had to move to astro 3.

@edvinasjurele
Copy link
Collaborator

Awesome! Thanks for helping me navigate your git-process :)

Do you have any plans for when the next release will be out? I'm guessing I'm not the only one who has had to move to astro 3.

It's out 🎉 https://www.npmjs.com/package/storyblok-rich-text-astro-renderer
Let me know if anything is missing. Happy coding!

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.

2 participants