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

feat: construct serverEnv, like we do with clientEnv #1000 #1021

Merged
merged 8 commits into from
Jan 8, 2023

Conversation

iduuck
Copy link
Contributor

@iduuck iduuck commented Dec 27, 2022

Closes #

✅ Checklist

  • I have followed every step in the contributing guide (updated 2022-10-06).
  • The PR title follows the convention we established conventional-commit
  • I performed a functional test on my final commit

Changelog

💯

@changeset-bot
Copy link

changeset-bot bot commented Dec 27, 2022

🦋 Changeset detected

Latest commit: 4596004

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

This PR includes changesets to release 1 package
Name Type
create-t3-app Minor

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

@vercel
Copy link

vercel bot commented Dec 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
create-t3-app ✅ Ready (Inspect) Visit Preview Jan 8, 2023 at 11:10AM (UTC)

@github-actions github-actions bot added 📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App labels Dec 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 100
🟢 Accessibility 100
🟢 Best practices 100
🟠 SEO 86
🟠 PWA 54

Lighthouse ran on https://create-t3-app-git-fork-ksv-vc-iduuck-issue-1000-t3-oss.vercel.app/

Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

We shouldnt need 2 separate objects right? Destruction can be in a single object shared with both server and client?

Ideally some comment saying you only need manual destruction for certain use-cases, and not require all envs to be destructured, i.e:

// schema.mjs
const destructured = {
  FOO: process.env.FOO,
  NEXT_PUBLIC_BAR: process.env.NEXT_PUBLIC_BAR,
}

const serverSchema = z.object({
  FOO: z.string(),
  BAZ: z.coerce.boolean(),
});

// server.mjs
serverSchema.parse({
  ...process.env,
  ...destructured
});

env.BAZ Will still work outside of next middleware

@iduuck
Copy link
Contributor Author

iduuck commented Dec 27, 2022

I get what you mean, @juliusmarminge. However, I think like this it could be misleading. What we could do, is using the clientEnv and extending the serverEnv with it.

Like:

const clientEnv = { /* ... */ };
const serverEnv = { ...clientEnv, BAZ: process.env.BAZ };

Also: We would be facing some issues with the validation on the middleware-side. When we only add the middleware environment variables one-by-one, it would be giving us schema validation errors, when using on the middleware (because – in your case – BAZ wouldn't be available at the runtime of the middleware).


Edit: Just saw the .coerce – however, this would be even more confusing, when the env variables would be "empty" in some cases.

@juliusmarminge
Copy link
Member

Fair points. I didnt think of that. I just want to make sure we provide as good DX as possible as many will think its annoying to do this many steps "just to get rid of | undefined 🤔

@iduuck
Copy link
Contributor Author

iduuck commented Dec 28, 2022

What | undefined are you referring to, the one in the declaration of serverEnv?

Btw: Do you know, why the build is failing with a wrong import (working for me locally).

@juliusmarminge
Copy link
Member

juliusmarminge commented Dec 28, 2022

What | undefined are you referring to, the one in the declaration of serverEnv?

So I guess where I'm going is if I need to do too much work to get it running in the first place, maybe I'd be fine with using process.env in edge-runtimes and just take the non-null assertion - or maybe we could get #404 working which would type it properly.

// in some edge function, e.g. 'src/middleware.ts'
const baz1 = process.env.BAZ;
//    ^? -> string | undefined


const baz2 = process.env.BAZ!;
//.   ^? -> string

Either would be fine since the variable have been validated to be present during build time.

Maybe I'm drifting here and this isn't a problem for others? I just wanna make sure we don't make it too much work to get environment variables to work or else people might just nuke it all and use process.env for everything 🤔

Btw: Do you know, why the build is failing with a wrong import (working for me locally).

You haven't defined the serverEnv object in the other schema files, just the base one.

@iduuck
Copy link
Contributor Author

iduuck commented Dec 28, 2022

I personally wouldn't want to use the non-null assertion throughout my application. Also for the purpose of the env validation, that t3 offers, I am sure there are use-cases, which would be awesome to have default's for.

So, just using process.env isn't feasable (again, personally).

@juliusmarminge
Copy link
Member

juliusmarminge commented Dec 29, 2022

Yea all valid points - curious how many apps use the middleware.ts file and if it's warranted to include this by default, or if we can just leave a comment about it? - the schema.mjs file is already kinda scary as it is 🤔 Although I guess this will get more and more relevant as edge runtime becomes more popular to use for more stuff... Maybe we should file an issue to https://github.com/vercel/next.js and see why this is the case - maybe they are open to change it?

@iduuck
Copy link
Contributor Author

iduuck commented Dec 30, 2022

I think this issue is giving us the reason.

This is related to how _middleware.ts is webpack transformed by the middleware-plugin.ts.
Only when you access an env var directly you will get the value. You can't destructure from process.env or get the whole object. I am not sure if this behaviour is intended or should be mentioned in the docs.

I think this is due to the nature of the DefinePlugin of webpack. However, with turbopack, I am curious, if this is working?

@iduuck
Copy link
Contributor Author

iduuck commented Jan 2, 2023

What's the plan to go for? Asking Next.js team first and implementing the PR for now? Or are we not wanting to implement the PR as is?

@juliusmarminge
Copy link
Member

What's the plan to go for? Asking Next.js team first and implementing the PR for now? Or are we not wanting to implement the PR as is?

I think this is the best approach even though I'm not a fan of the extra step - but that seems to be out of our hands unfortunately...

Go ahead and fix this up and we can ship it if no-one else has any objections? @c-ehrlich @nexxeln

@nexxeln
Copy link
Member

nexxeln commented Jan 3, 2023

What's the plan to go for? Asking Next.js team first and implementing the PR for now? Or are we not wanting to implement the PR as is?

I think this is the best approach even though I'm not a fan of the extra step - but that seems to be out of our hands unfortunately...

Go ahead and fix this up and we can ship it if no-one else has any objections? @c-ehrlich @nexxeln

Sounds good to me

@github-actions github-actions bot added the 📚 documentation Improvements or additions to documentation label Jan 5, 2023
@iduuck
Copy link
Contributor Author

iduuck commented Jan 5, 2023

OK, guys.

  • Updated the code at the docs (unfortunately, I am only speaking english of all the doc languages, so I was not able to change the text for the other languages)
  • Synchronized upstream
  • Fixed all issues from the tests

Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

👍👍

@juliusmarminge
Copy link
Member

Seems like you've disabled maintainers edit priviledges so i cant update your branch

@c-ehrlich
Copy link
Member

What's the plan to go for? Asking Next.js team first and implementing the PR for now? Or are we not wanting to implement the PR as is?

I think this is the best approach even though I'm not a fan of the extra step - but that seems to be out of our hands unfortunately...

Go ahead and fix this up and we can ship it if no-one else has any objections? @c-ehrlich @nexxeln

Bit late but I agree with this

@iduuck
Copy link
Contributor Author

iduuck commented Jan 8, 2023

Seems like you've disabled maintainers edit priviledges so i cant update your branch

How am I able to enable this? This is a repository from my company, so could be that this is the the case company-wide? Couldn't find a way to enable this?

--

Edit: updated manually

@juliusmarminge
Copy link
Member

Should be on the right side where you request reviews etc

@iduuck
Copy link
Contributor Author

iduuck commented Jan 8, 2023

CleanShot 2023-01-08 at 19 51 22@2x

unfortunately nothing there, would have activated this then ...

@juliusmarminge
Copy link
Member

It's there when you create the PR. Should be a way to update it after creation too??

CleanShot 2023-01-08 at 19 55 14

@juliusmarminge juliusmarminge mentioned this pull request Jan 17, 2023
3 tasks
devvianto605 pushed a commit to devvianto605/create-devviantex-nextjs-app-deprecated that referenced this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App 📚 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants