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

Add instructions if you run npm start #411

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

frankieroberto
Copy link
Contributor

@frankieroberto frankieroberto commented Nov 13, 2024

Currently if you run npm start, which is the common default start command for Node.js apps, then in the console you don’t see any messages and it looks like nothing is working. However it is working, just on the uncommon default port of 2000 (set in config.js).

This confused me and several others at first. The guidance to use npm run watch has since been added to the README but is easily missed.

This change aims to further help, by adding a console message saying:

Running at http://localhost:2000/

If you are working on this prototype now, press Control-C to stop the server and
type npm run watch instead to run the prototype in development mode.

This message is not shown when running npm run watch, as there the BrowserSync service will display the proxied localhost port to view the prototype on.

Currently if you run `npm start`, which is the common default start command for Node.js apps, then in the console you don’t see any messages and it looks like nothing is working.

However it is working, just on the uncommon default port of 2000 (set in `config.js`).

This adds a console message to say:

> Running at http://localhost:2000/
>
> If you are working on this prototype now, press Control-C to stop the server and
> type npm run watch instead to run the prototype in development mode.

This message is not shown when running `npm run watch`, as there the BrowserSync service will display the proxied localhost port to view the prototype on.
@joelanman
Copy link

this is our version in case it's helpful

https://github.com/alphagov/govuk-prototype-kit/blob/main/bin/cli#L194C1-L208C2

@frankieroberto
Copy link
Contributor Author

@joelanman

this is our version in case it's helpful

https://github.com/alphagov/govuk-prototype-kit/blob/main/bin/cli#L194C1-L208C2

Oh that’s nice! I didn’t think to suppress it when NODE_ENV=production - will do that.

Will switch to console.warn too.

@frankieroberto
Copy link
Contributor Author

@edwardhorsford @vickytnz does this seem sensible to you? should help anyone who runs npm start and then wonders if the kit is hanging...

Copy link
Contributor

@vickytnz vickytnz 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 the GOV.UK version is clearer as it makes it an actual warning?

This version confuses me as it has information I don't understand (like does node start mean i'm not running it in development mode? ):

if (
  process.env.WATCH !== 'true' // If the user isn’t running watch
  && process.env.NODE_ENV !== 'production' // and it’s not in production mode
) {
  console.info(`Running at http://localhost:${port}/`); // eslint-disable-line no-console
  console.warn('\x1b[36m \nIf you are working on this prototype now, press Control-c to stop the server and \ntype \x1b[34mnpm run watch\x1b[36m instead to run the prototype in development mode.\x1b[0m'); // eslint-disable-line no-console
}

Not sure if it needs Control+C, if so, a better version is to take the GOV.UK prototype kit version and then add 'Enter CTRL+C and ' making it

  ) {
    console.warn('Warning: It looks like you may have run the command `npm start` locally.')
    console.warn('Enter `CTRL+C` and then try running `npm run watch`')
    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()
  }

EDIT: oh wait, I understand that this is more of a generic catch - right now it just gives me errors if I do it wrong anyway (so much muscle memory for npm run dev) - but if this is about stopping the accidental npm start then i think it is better to be clear?

@frankieroberto
Copy link
Contributor Author

@vickytnz yeah this about stopping accidental npm start - which does work but just runs on an unexpected default port and doesn’t tell you which one. 🤦‍♂️

@frankieroberto
Copy link
Contributor Author

@vickytnz I’ve updated the text so that it now says this, which is a bit simpler:

Screenshot 2024-12-10 at 22 10 17

I’ve not included the bit about "it may be that your hosting service needs further configuration." as that should be unlikely - you’d have to have manually configured the host to use npm run watch or gulp as I don’t think it’s likely to default to that.

@frankieroberto
Copy link
Contributor Author

(I also removed the colours for now, even though they might make it slightly easier to read)

Copy link
Contributor

@vickytnz vickytnz left a comment

Choose a reason for hiding this comment

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

Run locally, works as expected

@frankieroberto frankieroberto merged commit de55c94 into main Dec 13, 2024
3 checks passed
@frankieroberto frankieroberto deleted the add-instructions-if-you-run-npm-start branch December 13, 2024 14:41
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.

3 participants