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

macros should not throw when running outside of vite #42

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pcattori
Copy link
Owner

Fixes #19

Copy link

changeset-bot bot commented May 24, 2024

🦋 Changeset detected

Latest commit: 65fd649

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

This PR includes changesets to release 1 package
Name Type
vite-env-only 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

Copy link
Collaborator

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Makes sense!

test/macros/macros.test.ts Outdated Show resolved Hide resolved
@pcattori
Copy link
Owner Author

pcattori commented May 25, 2024

Thinking more about this... I think a better API would be something like:

// script.ts
import { setEnv } from "vite-env-only/macros"

setEnv("server")
// in the future could even set compat for multiple envs:
// `setEnv("server", "client")`
// seems weird now, but with Vite v6 it fits right in

// ..run code that depends on macros..

where setEnv throws if run within Vite and also throws if called more than once in your script.
Then the macros (outside of Vite) can check that an environment was set and that it matches their intended env, otherwise throw.

The benefit here is that you would get an error that points directly to the offending code wrapped by the macro as soon as that definition is accessed. Whereas the current approach only gives you errors when you happen to access things that aren't defined in the env runtime and with stacktraces that point deep into your deps.

Copy link
Collaborator

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

What's the goal with the setEnv direction? To me the main goal is to provide feedback for those who haven't configured the Vite plugin properly within a Vite context. What's the use case outside of Vite?

@pcattori
Copy link
Owner Author

The issue with relying solely on the internal usingVite boolean is that it will let both clientOnly$ and serverOnly$ values through. I think the intent is actually for Node scripts to have access to values wrapped by serverOnly$ when running outside of Vite. It would still be nice to throw with a nice error if the script accidentally accessed a clientOnly$ value. That said, without the Vite plugin there won't be any DCE, so if you happen to import a client-only thing and just importing it is what causes Node to error out, then the setEnv stuff won't make that any better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't throw unreplaced macro when running without Vite
2 participants