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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃悶BUG]: import.meta.env.PROD has the wrong value, even if stubbed. #5525

Closed
6 tasks done
xsjcTony opened this issue Apr 12, 2024 · 15 comments 路 Fixed by #5590
Closed
6 tasks done

[馃悶BUG]: import.meta.env.PROD has the wrong value, even if stubbed. #5525

xsjcTony opened this issue Apr 12, 2024 · 15 comments 路 Fixed by #5590
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@xsjcTony
Copy link
Contributor

Describe the bug

import.meta.env.PROD will always have the value true.
It will only be false if I stub it to empty string '', but that's definitely not ideal.
Seems there's an issue when converting the string into a boolean (maybe !! is used straight away without checking for special value e.g. string 'false')
I don't have enough time to find the regression commit (it's working as expected before) and the exact logic in the code base, but please get this fixed, thanks馃檹

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-ckneff?file=test%2Fbasic.test.ts

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
    Memory: 5.85 GB / 31.69 GB
  Binaries:
    Node: 20.11.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.6.9 - ~\AppData\Local\pnpm\pnpm.EXE
  Browsers:
    Edge: Chromium (123.0.2420.81)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @vitejs/plugin-react-swc: ^3.6.0 => 3.6.0
    @vitest/coverage-istanbul: ^1.5.0 => 1.5.0
    @vitest/coverage-v8: ^1.5.0 => 1.5.0
    @vitest/ui: ^1.5.0 => 1.5.0
    vite: ^5.2.8 => 5.2.8
    vitest: ^1.5.0 => 1.5.0

Used Package Manager

npm

Validations

@xsjcTony xsjcTony changed the title import.meta.env.PROD has the wrong value, even if stubbed. [馃悶BUG]: import.meta.env.PROD has the wrong value, even if stubbed. Apr 12, 2024
@xsjcTony
Copy link
Contributor Author

I'm aware that in the environment variable it's always with type string, but I'm assuming it should follow Vite's behavior, as Vite is currently handling it to convert to 'false' to false

@sheremet-va sheremet-va added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Apr 13, 2024
@sheremet-va
Copy link
Member

sheremet-va commented Apr 13, 2024

This is technically a bug because setting a boolean to import.meta.env.PROD directly works correctly:

import.meta.env.PROD = true

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Apr 18, 2024

It looks like the default value of import.meta.env.PROD has changed in v1.5.0 and probably this is not intended?

https://stackblitz.com/edit/vitest-dev-vitest-vbi9xu?file=test%2Fbasic.test.ts

Show sample code
import { test, expect } from "vitest";

test("repro", () => {
  // v1.4.0
  [
    false,
    true,
    true,
    "",
    "1",
    "1"
  ];

  // v1.5.0
  [
    true,
    true,
    true,
    "false",
    "true",
    "1"
  ];

  console.log([
    import.meta.env.PROD,
    import.meta.env.DEV,
    import.meta.env.SSR,
    process.env.PROD,
    process.env.DEV,
    process.env.SSR,
  ]);
});

I'm assuming it should follow Vite's behavior, as Vite is currently handling it to convert to 'false' to false

I'm not sure if anything changed on the stubbing behavior (namely assigning "false" becoming true). This is implemented in #3944, so it's probably a different issue.

Btw, what do you mean by "Vite's behavior"? In Vite, import.meta.env is probably a constant and I thought people cannot do assignment.

@xsjcTony
Copy link
Contributor Author

I might be misleading, but what I mean is Vite seems correctly treating 'false' as false, since process.env is always of type string. But Vitest is not following the same way. But stubbing to 'false', it should return false instead of true

@hi-ogawa
Copy link
Contributor

Vite seems correctly treating 'false' as false

I still fail to understand what/when Vite is "treating" anything in terms of import.meta.env and process.env.
Are you referring to something like PROD=false vite ...? or NODE_ENV=production vite ...? Probably it would be easier if you can provide an example.

@xsjcTony
Copy link
Contributor Author

Sorry, maybe just forget that, the ultimate goal of this issue is to make sure Vitest treats import.met.env.PROD as false when it stubbed to 'false', since the stub function can only accept string value, which makes sense since process.env are all strings.

@hi-ogawa
Copy link
Contributor

Sorry, maybe just forget that, the ultimate goal of this issue is to make sure Vitest treats import.met.env.PROD as false when it stubbed to 'false', since the stub function can only accept string value, which makes sense since process.env are all strings.

Vitest currently handles this boolean/string problem by making '1' for true and empty string '' for false.

// process.env can have only string values and will cast string on it if we pass other type,
// so we are making them truthy
process.env.PROD ??= PROD ? '1' : ''
process.env.DEV ??= DEV ? '1' : ''

If there is something Vite already does, then Vitest aligning with it would totally makes sense, so that's why I was curious about that. Otherwise, current recommendation would be to stick with our internal representation, namely using an empty string vi.stubEnv('PROD', '') to make import.meta.env.PROD as false.

@xsjcTony
Copy link
Contributor Author

Yeah stubbing to empty string will definitely work, but that doesn't make sense. Imagine you are setting your env by cli, for example using cross-env, then you will have something like PROD=false, that way 'false' will also be the string version of false, if Vitest treat it as true, it's just so weird. I don't think things should working like that way.

@sheremet-va
Copy link
Member

PROD/DEV/SSR/MODE is not something that should be stubbed using PROD=false - this variable is set by Vite itself. If you are using cross-env, then you won't do that. PROD variable is based on the mode: https://vitejs.dev/guide/env-and-mode.html#env-variables

@xsjcTony
Copy link
Contributor Author

Yeah that makes sense, and I'm absolutely fine to change my test file to vi.stubEnv('PROD', '') instead of vi.stubEnv('PROD', 'false').
But it was working before, so it should be considered as a breaking change. I didn't see this is mentioned in the release note.

@sheremet-va
Copy link
Member

sheremet-va commented Apr 18, 2024

Not sure when you last updated, but I think it was always working like this. Under the hood stubEnv just sets process.env[name] = value.

You can still call import.meta.env.PROD = false and it will work correctly.

What we should do here is update the type signature and treat PROD/DEV/SSR differently like we already do in import.meta.env and allow booleans just for them:

export function createImportMetaEnvProxy() {

@xsjcTony
Copy link
Contributor Author

I tried with the exact vitest and vite version when that code is working in my codebase, but no luck, it's still breaking. No idea what's happening.
But yeah I agree that the best approach could be treating those special keys differently to allow boolean, which will resolve the issue and make more sense. Cheers!

@xsjcTony
Copy link
Contributor Author

xsjcTony commented Apr 19, 2024

Actually, I got what I was doing before.
Previously (at least on version v0.29.3, not sure which version it changed), the default value of import.meta.env.PROD is '', so it don't need to do any stubbing to that if I want import.meta.env.PROD to be falsy.
However, in a specific version the default value has been changed to 'false' (maybe), hence import.meta.env.PROD is now truhty by default, so I need to STUB it, then I find out stubbing to 'false' won't work. That's the root cause why I'm raising this issue.

@xsjcTony
Copy link
Contributor Author

In other word, if my memory is correct, import.meta.env.DEV and import.meta.env.PROD are BOTH true by default at the moment, which violates the definition of these 2 values that they should always be opposite to each other

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Apr 19, 2024

Actually, I got what I was doing before. Previously (at least on version v0.29.3, not sure which version it changed), the default value of import.meta.env.PROD is '', so it don't need to do any stubbing to that if I want import.meta.env.PROD to be falsy. However, in a specific version the default value has been changed to 'false' (maybe), hence import.meta.env.PROD is now truhty by default

Yeah, this matches with my reproduction in #5525 (comment) This started since v1.5.0 and it's likely a bug, so I made a PR to fix it #5561.

@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants