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

Make serve default command #1638

Merged
merged 3 commits into from
Oct 5, 2022
Merged

Make serve default command #1638

merged 3 commits into from
Oct 5, 2022

Conversation

lfdebrux
Copy link
Member

@lfdebrux lfdebrux commented Sep 30, 2022

We want to make sure that whatever service a user might be running their prototype online, they use the production scripts and configuration. Unfortunately, there isn't a standard way to detect whether a project is running in production or not; not all services set NODE_ENV [1], not all services will read a Procfile, etc. The only surefire way we can think of is to make it so that running npm start in a prototype runs the production code by default.

Unfortunately, this is a breaking change, as previously we told users to run npm start. We want to catch what we suspect will be a common case, where the user runs npm start having been used to that from before, and then ends up with a prototype that isn't working and no idea why. In those circumstances we should give the users a heads-up by outputting something to the terminal. While we can't be certain of when this might have happened, we can make a reasonable guess, and the downside of logging to a hosting services console erroneously is negligible.


Resolves #1202.

bin/cli Outdated
Comment on lines 104 to 110
console.warn('Warning: It looks like you may have run the command `npm start` locally')
console.warn('if you are using the kit on a personal machine you should use `npm run dev`')
console.warn()
console.warn('If you are seeing this warning in production, it may indicate that your')
console.warn('hosting service needs further configuration.')
console.warn()
Copy link
Member Author

@lfdebrux lfdebrux Sep 30, 2022

Choose a reason for hiding this comment

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

@NoraGDS to review this content.

Copy link
Contributor

Choose a reason for hiding this comment

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

whats the current logic to display this message?

Copy link
Member Author

@lfdebrux lfdebrux Oct 3, 2022

Choose a reason for hiding this comment

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

@joelanman I've added comments to the lines above to try and make the logic clearer, does that help?

Copy link
Member Author

@lfdebrux lfdebrux Oct 3, 2022

Choose a reason for hiding this comment

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

Paired with @NoraGDS on the message, our suggested revision to content:

Suggested change
console.warn('Warning: It looks like you may have run the command `npm start` locally')
console.warn('if you are using the kit on a personal machine you should use `npm run dev`')
console.warn()
console.warn('If you are seeing this warning in production, it may indicate that your')
console.warn('hosting service needs further configuration.')
console.warn()
console.warn('Warning: It looks like you may have run the command `npm start` locally.')
console.warn('try running `npm run dev`')
console.warn()
console.warn('If you see the above warning when trying to host your prototype online,')
console.warn('it may be that your hosting service needs further configuration.')
console.warn()

We still want more time to think about it though, ideally npm run dev would be the last thing the user sees, but swapping the order of the paragraphs around doesn't work either :/

@lfdebrux lfdebrux changed the base branch from ldeb-add-serve-script to v13 September 30, 2022 12:42
@nataliecarey
Copy link
Contributor

Tech side looks good to me. I also agree with the interface, I'd like confirmation from @joelanman on the approach:

npm run dev - runs the kit without password protection, watches for file changes and shows management pages.
npm start - runs the kit with password protection, doesn't watch for file changes and doesn't show management pages.

@joelanman
Copy link
Contributor

I agree npm start should run as we want it to online, for safety.

I'm still not sure about npm run dev - one of our issues is the kit being daunting to designers - feeling very 'developer-y', and this feels like the wrong direction for that issue. I don't see any obvious alternative though.

@joelanman
Copy link
Contributor

a point on management pages - we havent yet looked at that design in detail, and production/online will need access to Clear data (and Password, which is also in Manage)

@lfdebrux
Copy link
Member Author

I'm still not sure about npm run dev - one of our issues is the kit being daunting to designers - feeling very 'developer-y', and this feels like the wrong direction for that issue. I don't see any obvious alternative though.

Is the main concern the word dev in there? We could think about different words, npm run kit springs to mind. An alternative that just occurred to me, we make it so that running govuk-prototype-kit with no arguments starts the 'dev' server, and we tell users in future to use npx govuk-prototype-kit when running locally.

a point on management pages - we havent yet looked at that design in detail, and production/online will need access to Clear data (and Password, which is also in Manage)

I think we can we keep management pages out of scope for this PR, should be possible to fix later.

@lfdebrux lfdebrux linked an issue Oct 3, 2022 that may be closed by this pull request
@nataliecarey
Copy link
Contributor

I'm nervous about using the default command (npx govuk-prototype-kit) - I think it's useful to keep that for giving a summary of the commands available.

There's such a standard or npm run dev that I think we'd benefit from following that - I feel pretty strongly about that. Some designers will have played with other tools before or may find themselves using other tools later therefore I think they would benefit from us using more standard terms. Also designers with zero experience aren't our only users. My preferred options are:

a) npm run dev
b) npm run [something else]

@joelanman shall we catch up tomorrow about it?

@lfdebrux lfdebrux marked this pull request as ready for review October 5, 2022 10:30
We want to make sure that whatever service a user might be running their
prototype on online, they use the production scripts and configuration.
Unfortunately, there isn't a standard way to detect whether a project is
running in production or not; not all services set NODE_ENV [[1]], not
all services will read a Procfile, etc. The only surefire way we can
think of is to make it so that running `npm start` in a prototype runs
the production code by default.

[1]: #1202
We want to catch what we suspect will be a common case, where the user
runs `npm start` having been used to that from before, and then ends up
with a prototype that isn't working and no idea why. In those
circumstances we should give the users a heads-up by outputting
something to the terminal. While we can't be certain of when this might
have happened, we can make a reasonable guess, and the downside of
logging to a hosting services console erroneously is negligible.
@joelanman
Copy link
Contributor

good to discuss all this, agree that:

npm run dev

is the new local command to run the kit, and

npm start

is now reserved for running 'in production' - with a password

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-ldeb-npm-s-sppzmb October 5, 2022 15:04 Inactive
@lfdebrux lfdebrux merged commit 5bdfb98 into v13 Oct 5, 2022
@lfdebrux lfdebrux deleted the ldeb-npm-start branch October 5, 2022 15:43
@lfdebrux lfdebrux mentioned this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we make authentication check more robust?
4 participants