Fixes settings interpolation with dotenv variables #5544
Conversation
|
@arcanis thanks for the new .env support. Upgrading to rc.47 from rc.46 CI failed in a monorepo. I'm guessing env loaded by yarn leaks into the app/package (process.env) taking precedence in the way nextjs loads my .env file (exposed in global process.env). I might totally be wrong but preferred to ask you. PS: The example here is with ./apps/nextjs-app and the PR: belgattitude/nextjs-monorepo-example#3971 I admit the .env contains a incorrect value (-> a comment). But it might show some leakage |
|
That's the expected behaviour - |
|
Thanks @arcanis, In my experience it might cause issues with framework that rely on multiple files convention for loading envs.
Cause global process.env is taking precedence. I would recommend for safety to limit the env scope to internal yarn, otherwise I feel a lot of users might run into trouble. |
|
Those docs make me think it should be correct - One thing I'm not 100% sure is why Next.js didn't reject your .env before, since it was invalid as well, being empty? Perhaps our implementation should skip empty values, rather than set them to an empty string? Or is it a Next.js bug? |
|
Yes indeed, there's 2 causes
I like the idea of Ultimately I don't see how it would work when dealing with multiple frameworks in a monorepo for example. frameworkX would use .env.(staging|production|test) convention, frameworkB another one (.env.docker...). I'm in favour of making it opt-in, cause I feel there's a real risk in there |
|
Let's assume we have a However, with the current Berry and Next.js, the process is different. This change of load order might conflict with the expectations of the existing ecosystem and potentially lead to unforeseen issues. |
|
Yes, I can see that being annoying indeed 🤔 It'd be too bad to release such a nice feature and be met with negativity due to a harder migration path, even if technically a major is allowed to have breaking changes. In this case it's easy enough to make the change that I'll probably do it. My current thinking is to change the |
**What's the problem this PR addresses?** It was raised in #5544 (comment) that automatically injecting the `.env` file in the environment could break compatibility with some software that already do this in their own way. It makes sense, and I prefer to avoid doing that if there's a reasonable compromise. Also closes #5545. **How did you fix it?** Yarn will now default into loading the `.env.yarn` file, rather than `.env`. **Checklist** <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed.
|
Personally i really like the .env.yarn. wonderful idea. Thx @arcanis for the great you're doing. |
**What's the problem this PR addresses?** It was raised in yarnpkg/berry#5544 (comment) that automatically injecting the `.env` file in the environment could break compatibility with some software that already do this in their own way. It makes sense, and I prefer to avoid doing that if there's a reasonable compromise. Also closes yarnpkg/berry#5545. **How did you fix it?** Yarn will now default into loading the `.env.yarn` file, rather than `.env`. **Checklist** <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed.
|
Hi @arcanis. This worked great for resolving packages. Thanks! 💪 I'm curious if it's meant to also work for publishing? With ➤ YN0035: Response Code: 404 (Not Found)
➤ YN0035: Request Method: PUT
➤ YN0035: Request URL: https://registry.yarnpkg.com/@awesomelabs%2feslint-plugin-awesomelabsEven though I have npmScopes:
awesomelabs:
npmRegistryServer: https://gitlab.com/api/v4/packages/npm/
npmAuthToken: ${GITLAB_PACKAGE_REGISTRY_TOKEN}Publishing doesn't look like it's using the |
What's the problem this PR addresses?
I forgot in #4718 to add a test making sure that the yarnrc settings could use the variable set in the dotenv files (the existing tests were mainly around the values set during script execution), and of course I forgot a change 🥲
How did you fix it?
We now pass the updated environment when interpolating values.
Checklist