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

fix(templates/express): dynamically import chokidar so it doesn't crash production #7078

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Aug 5, 2023

prior to this change if you built the remix app, pruned dev only dependencies and attempted to npm start the server would bail as it tried to import chokidar

Closes: #

  • Docs
  • Tests

Testing Strategy:

@changeset-bot
Copy link

changeset-bot bot commented Aug 5, 2023

⚠️ No Changeset found

Latest commit: 2a0c53e

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

@MichaelDeBoey MichaelDeBoey changed the title fix: make it so chokidar isnt needed in production fix: make it so chokidar isn't needed in production Aug 6, 2023
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 think we should merge this into main + we should probably also update dev_v2 docs with this

@ngbrown
Copy link
Contributor

ngbrown commented Aug 8, 2023

This was also solved in #6922 (see review), along with making CJS easy to switch to and fixing a race condition.

@brophdawg11
Copy link
Contributor

As a template change I agree this should go to main - but we should hold off until v2 releases since I think the express server is pretty different in main (v1 dev server) and dev (v2 dev server).

@mcansh mcansh force-pushed the logan/chokidar-dev branch 2 times, most recently from 92fd4d5 to 7ad07bd Compare September 20, 2023 01:27
@mcansh
Copy link
Collaborator Author

mcansh commented Sep 20, 2023

rebased!

i originally opened this PR before v2 went out which is the reason for it being pointed to dev, not really in the mood to go through the whole git rebase --onto main dev (git branch --show-current) --committer-date-is-author-date shooting match to keep this PR but repoint it to main so if that's desired instead of merging + cherrypicking this one, i can close and bring it over to a new PR pointed to main

edit: the following did the trick without too much effort

pbcopy < ./templates/express/server.js
git checkout main
git pull
git checkout -
git reset main --hard
pbpaste > ./templates/express/server.js
git rebase --onto main dev (git branch --show-current) --committer-date-is-author-date
git push --force-with-lease

@mcansh mcansh changed the base branch from dev to main September 20, 2023 01:41
@mcansh mcansh changed the title fix: make it so chokidar isn't needed in production fix(templates/express): dynamically import chokidar so it doesn't crash production Sep 20, 2023
prior to this change if you built the remix app, pruned dev only dependencies and attempted to `npm start` the server would bail as it tried to import chokidar

Signed-off-by: Logan McAnsh <logan@mcan.sh>
@pcattori pcattori merged commit 3faf9d0 into main Sep 20, 2023
5 checks passed
@pcattori pcattori deleted the logan/chokidar-dev branch September 20, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants