-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(next): codegenDir and update .astro paths #11963
Conversation
🦋 Changeset detectedLatest commit: 1400894 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is blocked because it contains a major
changeset. A reviewer will merge this at the next release if approved.
For language-tools this mean that we need to load from multiple paths, here https://github.com/withastro/language-tools/blob/b79ff829224838492fcdfa273a03316863e724da/packages/ts-plugin/src/index.ts#L12 and here https://github.com/withastro/language-tools/blob/b79ff829224838492fcdfa273a03316863e724da/packages/language-server/src/nodeServer.ts#L45 |
Done in withastro/language-tools#951 |
Why are we moving all the files one-level down? Don't we control |
ATM we can't control where people put stuff. I'm in favor of creating somle kind of convention ( |
Do we want to say that it's safe to put your stuff in Astro owns the For |
It makes sense to me, as an integration author, to want to put codegen files in there since that's where astro does it. otherwise, that means you need to create your own As for the rest, I get your point, I guess we could move things outside of |
I think it's fine for integrations to put stuff into For |
Doesn't have to be in the beta so not urgent to answer. @Princesseuh when you have some time I'd like your opinion on that |
I don't see a big need for the PR to be done now, as it seems very unlikely for integrations to conflict with our files. Perhaps in the future if we see a lot of code gen being done. I think this can also somewhat be done in a non-breaking way? It's only the JSON schemas that are breaking I'm personally in favour of a dedicated |
Alright then if you're both okay with that, I'll do the following:
But yeah it doesn't have to be a 5.0 thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, with the little knowledge I have of injectTypes
when I read this changeset, I failed to understand what codegenDir
is for and how it is related to injectTypes
.
For example, the following phrase doesn't say anything to me. I don't understand what space you're referring to, and how other integrations are involved in this feature.
It allows you to have space without risking being overriden by another integration
I would try to help with the review, but the poor description of the PR and the changeset don't help.
Can I ask you to revisit the changeset, and make a ELI5 version of it?
@ematipico I clarified both PR desc and changeset, thanks for the feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a suggestion. In the changeset you highlight the fact that integrations now don't conflict with each other, and the hook provide the reserved directory to do so.
However, nothing blocks users from doing new URL("../other-integration", codegenDir)
, and that's how integrations can tamper with each other. Sure, integrations shouldn't do that.
Instead, I think we should be stricter and provide callbacks so we have finer control, for example:
const integration = {
name: 'my-integration',
hooks: {
'astro:config:setup': ({ writeToCodegenDir, purgeCodegenDir, readFromCodegeDir }) => {
purgeCodegenDir();
writeToCodegenDir(() => {
return { filename: "file.ts", contents: "{}" }
});
const map = await readFromCodegeDir();
for (const [filePath, contents] of map) {
}
}
}
}
Co-authored-by: Erika <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @florian-lefebvre ! Just some tiny language nits here, but looks great!
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
.astro/astro
to.astro
. My original plan in 4.14 was to move stuff from.astro
to.astro/astro
. I did that partially back then (for content, env and actions) but I revert it in this PR (see comments)createCodegenDir()
toastro:config:setup
. Points to.astro/integrations/<normalized_integration_name>
after being called. This is convenient for integrations authors to be able to put files in a folder where they're sure they can't override files generated by Astro (eg..astro/types.d.ts
), nor files generated by another integration. In 4.14, we introducedinjectTypes
which writes files to thiscodegenDir
directory already, so we just expose it nowTesting
Should still pass
Docs
Changeset + withastro/docs#9495