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

chore: use published image for docker-compose #623

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Dec 9, 2019

Impact: minor
Type: chore

Changes

When you clone this branch and run docker-compose up, it will now pull and use the latest published reactioncommerce/example-storefront:release-v3.0.0 image from DockerHub instead of node-dev. Host files are not linked in, NPM deps are not reinstalled, and NextJS will run in the faster production mode unless you set NODE_ENV=development in your .env file.

Breaking changes

Breaking only for devs who will now need to run ln -s docker-compose.dev.yml docker-compose.override.yml to get the previous behavior.

Testing

  • Verify dc up starts quickly and in prod mode. (You should not see anything about compiling pages in the logs.)
  • After dc down, run ln -s docker-compose.dev.yml docker-compose.override.yml and then dc up again. Verify it now starts in dev mode and reloads when you change files.
  • After dc down, run rm docker-compose.override.yml to remove the override file. It should be back to running in prod mode when you do dc up.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed aldeed added this to In progress in 3.0.0 via automation Dec 9, 2019
@aldeed aldeed moved this from In progress to Review in progress in 3.0.0 Dec 9, 2019
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@focusaurus
Copy link
Contributor

I'll give this a local test this afternoon.

@focusaurus
Copy link
Contributor

I seem some syntax errors but those aren't related to the docker mechanics here.

web_1  |  error  in ./src/pages/_error.js
web_1  |
web_1  | Syntax Error: SyntaxError: /usr/local/src/app/src/pages/_error.js: Support for the experimental syntax 'decorators-legacy' isn't currently enabled (23:1):
web_1  |
web_1  |   21 | });
web_1  |   22 |
web_1  | > 23 | @withStyles(styles)
web_1  |      | ^
web_1  |   24 | export default class Error extends Component {
web_1  |   25 |   static propTypes = {
web_1  |   26 |     classes: PropTypes.object,
web_1  |
web_1  |
web_1  |  @ multi ./pages/_error.js

@focusaurus
Copy link
Contributor

Wait hmm. It looks like the server starts and THEN does try to do some compiling:

web_1  | Server started ! ✓
web_1  |
web_1  |       http://localhost:4000
web_1  |       Press CTRL-C to stop
web_1  |
web_1  |  WAIT  Compiling...12:39:29 AM
web_1  |
web_1  | [12:39:29 AM] Compiling client
web_1  | [12:39:29 AM] Compiled client in 51ms
web_1  |  ERROR  Failed to compile with 2 errors12:39:29 AM
web_1  |
web_1  |  error  in ./src/pages/_app.js
w

Is that right for prod mode?

@aldeed
Copy link
Contributor Author

aldeed commented Dec 10, 2019

@focusaurus It shouldn't be doing that when you don't have the override file. Can you verify you don't have NODE_ENV=development in .env? That would also cause it. Even the prebuilt NextJS server will run in dev mode when NODE_ENV is development, which is always confusing to me.

@focusaurus
Copy link
Contributor

Yes, I had NODE_ENV=development in my .env. After commenting that out it behaves properly. Should we add a warning to the prod image to catch this? What percentage of our user base would hit the same scenario I did?

3.0.0 automation moved this from Review in progress to Reviewer approved Dec 10, 2019
Copy link
Contributor

@focusaurus focusaurus left a comment

Choose a reason for hiding this comment

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

Works as advertised I'm just wondering if there's lots of .env files out there in our developer base with NODE_ENV=development and whether we should try to guide them around that. That can be a separate issue though.

@aldeed aldeed merged commit d2590c8 into release-v3.0.0 Dec 10, 2019
3.0.0 automation moved this from Reviewer approved to Done Dec 10, 2019
@aldeed aldeed deleted the chore-use-published-image-for-dc branch December 10, 2019 23:04
@kieckhafer kieckhafer mentioned this pull request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants