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

Load configs with Vite when loading with Proload fails #4112

Merged
merged 8 commits into from
Aug 2, 2022
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 1, 2022

Changes

Testing

  • Test added

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Aug 1, 2022

🦋 Changeset detected

Latest commit: 7028fca

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

This PR includes changesets to release 13 packages
Name Type
astro Patch
@astrojs/mdx Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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

@matthewp matthewp marked this pull request as draft August 1, 2022 16:28
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) labels Aug 1, 2022
@matthewp matthewp changed the title Mdx plus ts Load configs with Vite when loading with Proload fails Aug 1, 2022
@matthewp matthewp assigned matthewp and unassigned matthewp Aug 1, 2022
@matthewp matthewp marked this pull request as ready for review August 1, 2022 18:11
Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

Suggestion on the config server, but I'm promerge on this one!

packages/astro/src/core/config.ts Outdated Show resolved Hide resolved
@@ -49,7 +49,7 @@ export default function mdx(mdxOptions: MdxOptions = {}): AstroIntegration {
// Workarounds tried:
// - "import * as remarkShikiTwoslash"
// - "import { default as remarkShikiTwoslash }"
(remarkShikiTwoslash as any).default,
(remarkShikiTwoslash as any).default ?? remarkShikiTwoslash,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good thinking here. Wonder if proload was somehow messing with the .default here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not why proload was failing. But this is needed to be compatible with loading in Vite.


return config as TryLoadConfigResult;
} catch(e) {
if (e instanceof ProloadError && flags.config) {
Copy link
Member

Choose a reason for hiding this comment

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

does this mean that --config won't work with .ts config files? Could this be moved to further down to be a final catchall, OR earlier in the function with a fs.stat call to check that the file exists before running proload or vite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this condition hits when the user passes a --config but that file doesn't exist. If the file does exist and is a .ts file it will either a) have worked through proload or b) caught here, but the instanceof check will fail and it will load through the subsequent Vite mechanism.

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM! Was there prior art to this strategy? It's clever but does it also mean that every dependency of your config file needs to run in Vite's SSR mode? Not sure how worried to be about that risk, but at the very least thankful that deps are all marked as external in vite 3.

@matthewp
Copy link
Contributor Author

matthewp commented Aug 1, 2022

Dependencies should be auto-external in Vite 3, so I would only assume that code from the local project needs to run in Vite's SSR mode. That's why I did it as a fallback because most of the time (I guess if you're not using mdx) it will load through the normal proload flow.

I think maybe the ideal fix here would be to skip both proload and Vite loading and just run the source through esbuild to remove types and then import() it, so that everything goes through normal Node.js ESM.

@matthewp matthewp merged commit e33fc9b into main Aug 2, 2022
@matthewp matthewp deleted the mdx-plus-ts branch August 2, 2022 12:08
@astrobot-houston astrobot-houston mentioned this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Using @astrojs/mdx with astro.config.ts fails to start
3 participants