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

feat: add --prod flag to dev command #5338

Merged
merged 31 commits into from
Jan 9, 2023
Merged

feat: add --prod flag to dev command #5338

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
087f7dd
feat: add `--serve` flag to `dev` command
eduardoboucas Dec 23, 2022
b3f582d
refactor: simplify internal directory clean up
eduardoboucas Dec 23, 2022
90402fd
refactor: load built functions
eduardoboucas Jan 2, 2023
d5652e7
refactor: make things nicer
eduardoboucas Jan 2, 2023
ac6a845
refactor: add const
eduardoboucas Jan 2, 2023
134bf3f
feat: show debug message
eduardoboucas Jan 3, 2023
d782283
refactor: rename variable
eduardoboucas Jan 3, 2023
7a0d578
Merge branch 'main' into feat/serve-command
eduardoboucas Jan 3, 2023
5411c3c
chore: add `extract-zip` dependency
eduardoboucas Jan 3, 2023
05e9dc3
chore: update snapshots
eduardoboucas Jan 3, 2023
3ec828b
refactor: conditionally set `quiet` flag
eduardoboucas Jan 3, 2023
1a89050
refactor: simplify code
eduardoboucas Jan 3, 2023
2d8d2d0
chore: update docs
eduardoboucas Jan 3, 2023
78c35b6
refactor: rename flag to `prod`
eduardoboucas Jan 3, 2023
0bcc9a2
refactor: update message
eduardoboucas Jan 3, 2023
4090013
feat: do not set `NETLIFY_DEV` when `prod` is set
eduardoboucas Jan 3, 2023
9be736b
chore: remove lint exceptions
eduardoboucas Jan 3, 2023
e3eea4a
Apply suggestions from code review
eduardoboucas Jan 3, 2023
2934233
refactor: treat `NETLIFY_DEV` consistently across function types
eduardoboucas Jan 3, 2023
ebb2334
refactor: add `internal` source for env vars
eduardoboucas Jan 3, 2023
01df8a3
fix: handle internal vars correctly
eduardoboucas Jan 3, 2023
59297c1
chore: update docs
eduardoboucas Jan 3, 2023
917fc13
refactor: remove unused variable
eduardoboucas Jan 3, 2023
ae74853
feat: set `context=production` when `prod` is used
eduardoboucas Jan 4, 2023
f3cc600
refactor: add beta label
eduardoboucas Jan 4, 2023
f4a2dfd
Merge branch 'main' into feat/serve-command
eduardoboucas Jan 9, 2023
dbd9dcc
chore: remove ESLint directive
eduardoboucas Jan 9, 2023
bc5c59d
fix: fix imports
eduardoboucas Jan 9, 2023
496783a
refactor: always start functions server
eduardoboucas Jan 9, 2023
a92f755
refactor: ensure internal functions directory
eduardoboucas Jan 9, 2023
af00758
refactor: reinstate functions server check
eduardoboucas Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,11 @@ module.exports = {
],
},
},
{
files: ['tests/**/*'],
rules: {
'require-await': 'off',
},
},
],
}
1 change: 1 addition & 0 deletions docs/commands/dev.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ netlify dev
- `live` (*boolean*) - start a public live session
- `offline` (*boolean*) - disables any features that require network access
- `port` (*string*) - port of netlify dev
- `prod` (*boolean*) - build the site for production and serve locally
Copy link
Contributor

Choose a reason for hiding this comment

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

Just creeping on your PR 😄 How does this work with or in contrast to the context flag (documented above), that you can also set to production ?

ie netlify dev --context production

Copy link
Contributor

Choose a reason for hiding this comment

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

(the CLI doc above mentions only environment variables, but our docs site makes it sound like using the context flag builds your site for production - so this would need to be updated if that's not correct)

Copy link
Member Author

Choose a reason for hiding this comment

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

Your creeping is very much welcome and that's a great point! The CLI docs are correct: the context flag only affects environment variables. This makes me wonder whether we should repurpose context and introduce this new behaviour (i.e. actually building the site for production) when context is production. And we could make --prod an alias for --context=production.

I think that would make things more consistency, with just two concerns worth considering:

  1. That would make this a breaking change, since we'd be changing an existing flag rather than introducing a new one
  2. This new behaviour means that watching for files won't work, and people will have to restart the CLI for the build to be run again and their changes to be picked up. I worry that it might be confusing that a specific value of context triggers this behaviour, rather than an entirely new flag.

But if we decide not to reuse context, it'll be a bit difficult to explain why you have --prod and --context=production.

@ascorbic What are your thoughts on this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this a bit more and I feel that a more manageable step would be to keep context as is (i.e. just about environment variables) and make prod automatically set --context=production, so that you're building the production site and using the production env vars.

Copy link

Choose a reason for hiding this comment

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

Now that we have named the flag prod, I agree that making it automatically set --context=production feels most intuitive

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I sugegsted having it as a separate command. Using ntl dev when it's building for production is a little unintuitive

Copy link
Member Author

Choose a reason for hiding this comment

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

Building a separate command is a bigger lift (if done properly) that unfortunately I don't have the capacity for right now. I've pushed ae74853 which sets --context=production when --prod is set, and in f3cc600 added a "beta" label to the flag in the help menu. We do this for other commands.

I suggest that we ship this as is and then iterate on it. If we come to the conclusion that this makes more sense as a separate command, we can break it out and deprecate --prod.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is a good compromise. Does our CLI lib support aliases? If so, that would probably be the simplest solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

#5351 turns the flag into a separate command. We can get it merged after this one.

- `sessionId` (*string*) - (Graph) connect to cloud session with ID [sessionId]
- `targetPort` (*string*) - port of target app server
- `debug` (*boolean*) - Print debugging information
Expand Down
73 changes: 73 additions & 0 deletions npm-shrinkwrap.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@
"execa": "^5.0.0",
"express": "^4.17.1",
"express-logging": "^1.1.1",
"extract-zip": "^2.0.1",
"fastify": "^4.10.2",
"find-up": "^5.0.0",
"flush-write-stream": "^2.0.0",
Expand Down
Loading