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

Starting or Building Keystone with NODE_ENV set production fails. #8774

Open
chukwumaijem opened this issue Aug 20, 2023 · 18 comments
Open

Starting or Building Keystone with NODE_ENV set production fails. #8774

chukwumaijem opened this issue Aug 20, 2023 · 18 comments
Labels
🐛 bug Unresolved bug

Comments

@chukwumaijem
Copy link

Please add steps to reproduce the problem

  • Scaffold a new Keystonejs app.
  • Install dotenv or a similar tool
  • inside keysone.ts file, import dotenv import 'dotenv/config';
  • set NODE_ENV=production in .env file
  • Try building the project.

Please describe what you expected to happen

It should build successfully. It builds in development, testing, and staging. It builds even when NODE_ENV is undefined. But only fails in production mode.

Please add any screenshots if possible

image

Please add contextual information such as your node version (node -v), or the web browser you used

Tried on NODE v16, v18, and v20 (using nvm).

@chukwumaijem
Copy link
Author

I just downgraded to "@keystone-6/core": "^4.0.1", then "@keystone-6/core": "3.1.3" and got the exact same behavior.

@cyanonoob
Copy link

Had the same, removed node_env from my .env and building was fine. Cryptical, and is manually setting node_env really that evil?

@chukwumaijem
Copy link
Author

Had the same, removed node_env from my .env and building was fine. Cryptical, and is manually setting node_env really that evil?

If you set it to any value other than production. it will build successfully. But why is setting it to production a problem?

@dcousens
Copy link
Member

Cannot reproduce this, and you should always have NODE_ENV=production if you aren't locally developing the project, @chukwumaijem please provide more information

@chukwumaijem
Copy link
Author

The only other information missing from what I already provided is that I'm working on Ubuntu 22.04. I can make a loom showing everything I did step by step.

Noticed this because it failed during image build on the CI with NODE_ENV=production, tried the same env locally and got the same error.

@marekryb
Copy link
Contributor

@dcousens I checked this and I can reproduce it. The key is to import dotenv before importing schema (otherwise error does not happen) and having NODE_ENV=production in .env
Funnily enough if you use NODE_ENV=production yarn keystone build then it works fine (with and without dotenv). Also any other env loaded by dotenv seem to work just fine.
Which leads me to believe this is somehow related to esbuild special treatment of NODE_ENV as per GHSA-25mx-2mxm-6343

@chukwumaijem if this is blocker for you (you use NODE_ENV checks for some other code), try adding NODE_ENV in the package.json script commands - "build": "NODE_ENV=production keystone build", "dev": "NODE_ENV=development keystone dev" etc.

@chukwumaijem
Copy link
Author

Thank you @marekryb. It builds successfully when I set NODE_ENV in the build script.

@dcousens
Copy link
Member

The key is to import dotenv before importing schema (otherwise error does not happen) and having NODE_ENV=production in .env

This aligns with the dotenv documentation

As early as possible in your application, import and configure dotenv

@chukwumaijem
Copy link
Author

That's exactly what I was doing. See the screenshot attached. Or the loom. But it was causing an error.

@dcousens
Copy link
Member

Interesting, I think we should keep this open

@dcousens dcousens reopened this Aug 24, 2023
@dcousens dcousens added the 🐛 bug Unresolved bug label Aug 24, 2023
@ttbarnes
Copy link
Contributor

I'm getting the same issue - created an issue before I found this one

@dcousens
Copy link
Member

@ttbarnes are you saying that setting NODE_ENV=production in your environment fixed #8791 ?

@ttbarnes
Copy link
Contributor

@dcousens It only works in npm scripts as per suggestion above, having it set in a .env file doesn't work.

I don't know the underlying code, probably a separate discussion, but I do wonder if getAdminMetaInRelationshipField should even be called when the --no-ui flag is passed, as there'd be no admin UI?

@dcousens
Copy link
Member

dcousens commented Aug 31, 2023

@ttbarnes in a major release coming soon (as it will be a breaking change), we will be updating the behaviour of ui.isDisabled to omit the definition of the adminMeta GraphQL queries completely when true, this might be nearer to what you expect.

As recommended in #8773.

@ttbarnes
Copy link
Contributor

Thanks @dcousens that sounds fantastic

@marekryb
Copy link
Contributor

marekryb commented Sep 13, 2023

@dcousens
I looked into it a bit, and the key information was hidden in the plain sight

at Object.getAdminMetaForRelationshipField ([...]/@keystone-6/core/dist/create-admin-meta-5d6c7c4d.cjs.prod.js:154:11)
at Object.getAdminMeta ([...]/@keystone-6/core/fields/dist/keystone-6-core-fields.cjs.prod.js:2882:45)
at Object.createAdminMeta [...]/@keystone-6/core/dist/create-admin-meta-d9f6d1a4.cjs.dev.js:142:245)

Notice how initially node is using .dev.js bundle and later it is switching to .prod.js bundle.

So here is what I believe is happening:

  1. Unless you have NODE_ENV specified in your initial env, build process starts with NODE_ENV undefined. By default node assumes development environment and selects .dev.js bundle.
  2. On build::getBuiltKeystoneConfiguration it dynamically loads .keystone/config.js, which in turn makes dotenv set NODE_ENV to your value (production).
  3. Because all initial imports are already resolved, it keeps using them (.dev.js), but any new import will use .prod.js bundle.
  4. It just happens that create-admin-meta.ts is using global variable currentAdminMeta (proabably should be refactored) that gets initialized in .dev.js import, but then the same file gets re-imported from .prod.js and now this variable is undefined.
  5. Error happens

Additional (surprising) takeaway:
Prisma is already using dotenv. Although keystone is not using prisma init, as far as I understand prisma client is reading from .env when creating instance anyway. .keystone/config.js already has dotenv code whether you want it or not. However because it is not executed on require call, there are no build problems.

@dcousens
Copy link
Member

dcousens commented Feb 8, 2024

Thanks for the detailed description @marekryb, hopefully we can reduce the complexity of this through some parallel improvements and then come back to thinking about how we can stop this from happening.

Using NODE_ENV=production ... at each stage of your deployment is recommended in any event.

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

No branches or pull requests

5 participants