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.

Sign up for GitHub

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

Jump to bottom

feat(templates/express): support .env durning local development #4017

Merged
merged 4 commits into from Nov 1, 2022
Merged

feat(templates/express): support .env durning local development #4017

merged 4 commits into from Nov 1, 2022

Conversation

lili21
Copy link
Contributor

@lili21 lili21 commented Aug 18, 2022

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2022 •
edited

⚠️ No Changeset found

Latest commit: eaa76d6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Aug 18, 2022

Hi @lili21,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

machour
machour requested changes Aug 19, 2022
View reviewed changes
Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

CLA needs to be signed correctly before this PR is reviewed by the team.

contributors.yml Outdated Show resolved Hide resolved
templates/express/server.js Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey changed the title support .env durning local development feat(templates/express): support .env durning local development Aug 19, 2022
MichaelDeBoey
MichaelDeBoey reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

I don't think we want to merge this, as this seems very specific and people can do this in their own project if they want to.

Will let the team (@kentcdodds & @mcansh) make the final decision.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Aug 21, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@lili21
Copy link
Contributor Author

lili21 commented Aug 22, 2022

the express template is created by the remix team. I guess the developer would expect that the DX is the same as the default template.
things like local environments and live reload should be supported out of the box, right?

MichaelDeBoey
MichaelDeBoey requested changes Aug 24, 2022
View reviewed changes
templates/express/package.json Outdated Show resolved Hide resolved
@MichaelDeBoey MichaelDeBoey requested review from machour and removed request for mcansh August 24, 2022 16:49
@MichaelDeBoey MichaelDeBoey assigned mcansh and unassigned kentcdodds Aug 25, 2022
lili21 and others added 2 commits August 25, 2022 09:57
machour
machour previously requested changes Sep 3, 2022
View reviewed changes
templates/express/package.json Show resolved Hide resolved
machour
machour reviewed Sep 4, 2022
View reviewed changes
Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

I'm not fan of this change, but I'll leave it to the team to decide since it's not my place.

My 2cts: The express adapter is meant as "do what you want if you feel Remix App Server doesn't meet your needs", and it should be as bare-bone as possible.

@machour machour dismissed their stale review September 4, 2022 13:54

the requested change have been made

@mcansh
Copy link
Collaborator

mcansh commented Sep 7, 2022 •
edited

The express adapter is meant as "do what you want if you feel Remix App Server doesn't meet your needs", and it should be as bare-bone as possible.

I agree 100% - templates are made to be as basic as possible, however i do think this should be an "example" which you could then create with npx create-remix@latest --template examples/<name>

@kiliman
Copy link
Collaborator

kiliman commented Sep 7, 2022

Yes, if you need dotenv, it'll be easy to update the server.js file manually after setup.

Also, if/when PR #4123 is merged, you'll get the DX of Remix App Server automatically. With the customization of Express.

@SokratisVidros
Copy link

I came across the same issue today and after some digging I patched server.js to load the .env file. However, it is expected to have the same experience across all official remix stacks. +1 on merging this.

@mcansh
Copy link
Collaborator

mcansh commented Nov 1, 2022

the more i think about this, the more i think we should include it. does it work fine if there is no .env as well?

@SokratisVidros
Copy link

the more i think about this, the more i think we should include it. does it work fine if there is no .env as well?

Yes

@MichaelDeBoey MichaelDeBoey merged commit c98b06d into remix-run:main Nov 1, 2022
@lili21 lili21 deleted the template-express branch November 4, 2022 02:28
midgleyc pushed a commit to midgleyc/remix that referenced this pull request Nov 15, 2022
@lili21 @MichaelDeBoey @midgleyc
feat(templates/express): support .env durning local development (re…
8994cd5 
…mix-run#4017)

* support dotenv durning local development

* Update templates/express/package.json

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* chore: specify dotenv dep

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: lili.21 <lili.21@bytedance.com>
kentcdodds pushed a commit that referenced this pull request Dec 15, 2022
@lili21 @MichaelDeBoey @kentcdodds
feat(templates/express): support .env durning local development (#4017
34515d9 
)

* support dotenv durning local development

* Update templates/express/package.json

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>

* chore: specify dotenv dep

Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: lili.21 <lili.21@bytedance.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machour machour machour approved these changes

@MichaelDeBoey MichaelDeBoey MichaelDeBoey approved these changes

@mcansh mcansh mcansh approved these changes

Assignees

@mcansh mcansh

Labels
CLA Signed template:express
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@lili21 @mcansh @kiliman @SokratisVidros @machour @MichaelDeBoey @kentcdodds