Remove shamefully-hoist #4842
Remove shamefully-hoist #4842
Conversation
🦋 Changeset detectedLatest commit: d7afd58 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 |
github-actions
bot
added
pkg: astro
| /** Normalize URL to its canonical form */ | ||
| export function createCanonicalURL(url: string, base?: string): URL { | ||
| let pathname = url.replace(/\/index.html$/, ''); // index.html is not canonical | ||
| pathname = pathname.replace(/\/1\/?$/, ''); // neither is a trailing /1/ (impl. detail of collections) | ||
| if (!npath.extname(pathname)) pathname = pathname.replace(/(\/+)?$/, '/'); // add trailing slash if there’s no extension | ||
| if (!getUrlExtension(url)) pathname = pathname.replace(/(\/+)?$/, '/'); // add trailing slash if there’s no extension |
There was a problem hiding this comment.
Question: could we use path.extname from node here? I'm not sure since it imported path-browserify before (maybe just a auto-import issue).
Context: path-browserify was imported by astro-rss before but was not declared as dependency. Thought we could simplify remove the package with a simpler replacement.
| // NOTE: Sometimes, tests fail with `TypeError: process.stdout.clearLine is not a function` | ||
| // for some reason. This comes from Vite, and is conditionally called based on `isTTY`. | ||
| // We set it to false here to skip this odd behavior. | ||
| process.stdout.isTTY = false; |
There was a problem hiding this comment.
I hit this special case locally where e2e is failing on process.stdout.clearLine for some reason. It's not related to the shamefully-hoist change from what I can tell.
| ssr: { | ||
| external: ['@astrojs/mdx', '@astrojs/react'], | ||
| }, |
There was a problem hiding this comment.
Added these packages to tryLoadConfig. There's no way to externalize everything always in Vite, so this was the best I could find fixing this.
NOTE: there's a comment explaining this above the highlight change
|
@bluwy have you tried this outside of the monorepo? If not we can create a preview release for this in order to test it out. |
|
Yeah I've tested this with https://github.com/bluwy/whyframe/tree/master/docs and the basics template + Svelte/Lit framework locally, which seems to work. The complex part of the PR is getting strict hoist to work in Astro itself, but getting strict hoist to work in user projects is simpler. The main takeaway that maybe we should document is that:
The rest should be working as is. |
Changes
shamefully-hoist=truein the Astro codebase and user project.@astrojs/rssand@astrojs/markdown-remark.Testing
All tests should pass
Docs
N/A. It doesn't look like the docs touch on
shamefully-hoist, so I think this change could be made transparently.