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.

Sign up for GitHub

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

Jump to bottom

fix(deno): do not set vite.ssr.noExternal #8652

Merged
merged 2 commits into from
Sep 25, 2023
Merged

fix(deno): do not set vite.ssr.noExternal #8652

merged 2 commits into from
Sep 25, 2023

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Sep 23, 2023

Changes

Testing

Could use more tests, not sure if this is the time for it.

Docs

Does not affect usage.

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 91473c8

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 github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 23, 2023
@lilnasy lilnasy mentioned this pull request Sep 23, 2023
Closed
@lilnasy lilnasy marked this pull request as ready for review September 23, 2023 19:05
@lilnasy lilnasy changed the title fix(deno): safely set external specifiers fix(deno): safely set non-external specifiers Sep 23, 2023
lilnasy
lilnasy commented Sep 23, 2023
View reviewed changes
packages/integrations/deno/src/index.ts
Comment on lines -152 to -154
vite.ssr = {
noExternal: COMPATIBLE_NODE_MODULES,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

noExternal was set to true originally. I had to change it to the list of bare node built-in modules in #7288. But apparently, it doesn't need to be set at all.

@lilnasy lilnasy changed the title fix(deno): safely set non-external specifiers fix(deno): do not set vite.ssr.noExternal Sep 23, 2023
@ematipico
Copy link
Member

ematipico commented Sep 25, 2023 •
edited
Loading

Are we sure about this fix? The adapter is about to be moved #8559, and I don't think we plan to do a new release

@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 25, 2023

I know but unfortunately, this is a build-breaking bug.

@bluwy
Copy link
Member

bluwy commented Sep 25, 2023

The changes looks fine to me. I think the previous implementation probably wasn't affecting much either, but I don't quite understand how this PR fixes #8390. How was noExternal-ing the node modules causing astro:* imports to be left in the built bundle before?

@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 25, 2023 •
edited
Loading

beats me 🤷

ematipico
ematipico approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@ematipico ematipico left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Checked with @matthewp on Discord and we are good to go with merging this PR. Let's 🚢 it!

@lilnasy lilnasy merged commit 954cadc into withastro:main Sep 25, 2023
@astrobot-houston astrobot-houston mentioned this pull request Sep 25, 2023
Merged
@lilnasy lilnasy mentioned this pull request Sep 25, 2023
Merged
@lilnasy lilnasy deleted the fix-deno-8390 branch September 30, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Astro 3 - build failed with Deno adapter and React integration
3 participants
@lilnasy @ematipico @bluwy