Conversation
* Try mocha/chai test runners * Disable failing smoke test for now Will revert when next can build docs * Enable mocha in parallel mode * Remove warning * Update docs * Fix Windows bug * Fix internal imports * Fix styles
* fix(logger): locale parsing * Fixed issue of compiler crash when "c" locale was encountered * Return default locale if parsed locale is less than 2 chars long * chore: add changeset
* Add back in support for children * Be more careful
* Enables most slot tests * Use spreadAttributes
* [next] Support for custom elements * Fix eslint errors * eslint again
* fix Astro.fetchContent * fix(fetchContent): cast type Co-authored-by: Nate Moore <nate@skypack.dev>
* Move hydration to the compiler * Move extracting url, export to util fn
* Implements top-level Astro + Astro.resolve * Fix linting
* chore: update vite * fix(renderers): point renderers to resolved server/client entrypoints
|
|
sgtm |
lol I thought you would have more feedback! If this sounds like an improvement we can try it out, but I’m also happy for this not to be the final structure and for others to have say |
natemoo-re
left a comment
There was a problem hiding this comment.
Very in favor of all of these changes!
A few small notes.
- IIUC I'm pretty sure
packages/astro/src/core/ssr/{html,h}.tscan both be removed. They're holdovers from Astro Classic™ - It'd be great to add a
README.mdfile to each directory to repeat what kind of code lives in this directory, as you've outlined in this PR. Useful to colocate that info with the code and it will showup on GitHub as folks are browsing the repo.
Cool. I wasn’t sure if we still needed them or not as “the default/fallback renderer” but you’re right—our compiler spits out valid HTML now! And we have that |
natemoo-re
left a comment
There was a problem hiding this comment.
Looks great! Thanks for updating 🎉
| '@typescript-eslint/explicit-module-boundary-types': 'off', | ||
| '@typescript-eslint/no-empty-function': 'off', | ||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| '@typescript-eslint/no-non-null-assertion': 'off', |
Not really, sorry. I'm pretty tolerant of any organization as long as things aren't so deeply nested that it's hard to find files without search, and this one seems fine in that regard. I'm pretty unopinionated on where files live otherwise. |
|
Feedback incorporated! Closing this because of the gnarly rebase in favor of #1569. |
Changes
This is quite a bikeshed, but wanted to get some feedback. Talked this over with Nate and we’d like to take a little time to organize the structure a bit better before committing to it.
Background
The
nextbranch has changed a lot since the first cut of Astro! Though the core principles remain the same, I think we struggled to determining which code ran where (I know I did). Given any code file, it was difficult to determine which context it ran in, and at what point in the chain.This seems to be even more important in Vite-land, where running code inside Vite executes more differently than top-level Astro (especially given then current ESM / SSR problems).
This PR
This PR proposes the following rules, inspired by SvelteKit and Vite’s:
@types/: self-explanatorycli/: self-explanatory.core/: it’s easy to just say “this is Astro core” but that’s vague. This is code that runs in the top Node context. Nothing here gets run inside Vite SSR, nor does it contain browser code. It contains a few subfolders:build/,dev/,preview/: the main actions of Astro. Somewhat self-explanatoryssr/: this sets up Vite SSR and compiles.astrofiles.runtime/: this was a bit of a “catchall” before but I’d like to propose that runtime now only means code that runs in a different context. It contains 2 subfolders, for the 2 contexts:client/: this code executes in the browser. It contains hydrations for JS.server/: this code gets called withinvite.ssrLoadModule(). So anything in here will get run inside Vite.vite-plugin-*: the Vite plugins were moved out into their own folders. Perhaps eventually we’ll release these as separate packages, but that’s just hassle for now. Still, I think it would be good to try and isolate these Vite plugins not only for possible future release, but also debugging.All the existing code was moved into one of these folders.
Put another way, it solves the following issues both from
nextas well as some frommain:internal/: this was fine, however, at different stages this would pull in files from other contexts which meant this wasn’t the only stuff that would get executed within SSRruntime/: this was a muddle of code executing in all different contexts. Code that would load the SSR, and code that would execute within SSR, would be side-by-side. That means you’d have to watch your imports!Testing
Tests should pass
Docs
After
nextgets merged, we can probably update CONTRIBUTING.md with some of this info (or, I’m partial to README files within folders myself, even if they’re just stubs explaining the goal of a folder)