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

fix: import.meta.env.BASE_URL will be '/' in client loaded component on dev mode #4886

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

yuhang-dong
Copy link
Contributor

Changes

Hi, I have found that import.meta.env.BASE_URL will be set / in client loaded component on dev mode.

And after searching from issues, I found #3567 describe the same situation.

In the conversion, I have found pr#3955 just fix prod mode.

You can visit the below link to see import.meta.env.BASE_URL will be set '/' in client loaded component on dev mode:
https://stackblitz.com/edit/github-vhnc6s-13dn4n?file=src%2Fpages%2Findex.astro
截屏2022-09-27 21 11 56

So, I defined the import.meta.env.BASE_URL to setting.config.base.

Testing

This situation need e2e test, so I added it on [packages/astro/e2e/astro-envs.test.js]

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2022

🦋 Changeset detected

Latest commit: e227d9f

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: astro Related to the core `astro` package (scope) label Sep 27, 2022
@@ -52,6 +52,9 @@ export default async function dev(
optimizeDeps: {
include: rendererClientEntries,
},
define: {
'import.meta.env.BASE_URL': settings.config.base ? `'${settings.config.base}'` : 'undefined',
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be the same as the following?

settings.config.base || 'undefined'

Copy link
Contributor

Choose a reason for hiding this comment

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

If base is not set, would it not show /undefined/ instead of / in that case here? Since you added undefined in strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If base is not set, it would show / in that case here.

What's more, settings.config.base and '${settings.config.base}' are different, notice the single quotes

Copy link
Contributor

Choose a reason for hiding this comment

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

@yuhang-dong yup, now I see it. Thanks for the accurate pointers!

Copy link
Contributor

@rishi-raj-jain rishi-raj-jain left a comment

Choose a reason for hiding this comment

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

LGTM

@rishi-raj-jain
Copy link
Contributor

rishi-raj-jain commented Sep 27, 2022

@yuhang-dong

I don't think I'm eligible to post a 'approved'

@matthewp matthewp merged commit 61d26f3 into withastro:main Oct 4, 2022
@astrobot-houston astrobot-houston mentioned this pull request Oct 4, 2022
@bluwy bluwy mentioned this pull request Oct 5, 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants