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

first local test run requires npm run build #222

Closed
zeke opened this issue Oct 8, 2020 · 9 comments
Closed

first local test run requires npm run build #222

zeke opened this issue Oct 8, 2020 · 9 comments
Labels
engineering Will involve Docs Engineering good first issue Good for newcomers help wanted Anyone is welcome to open a pull request to fix this issue

Comments

@zeke
Copy link
Contributor

zeke commented Oct 8, 2020

A fresh clone of this repo requires npm run build to be run before all the tests will pass. This is an easy thing to forget because you only have to run it once and then npm test continues to work without a fresh build.

One way we can work around this is by adding a pretest script to package.json with a value of npm run build. That way the build will always run first when anyone runs npm test.

That would solve the immediate problem for first-time contributors, but it would also incur a performance penalty every time you run the tests locally, as the build currently takes ~4 seconds to run on my superfast computer. Maybe 4s doesn't matter much in the grand scheme of things... but it would be nice to not incur that cost.

🤔 What are some ways we can solve this?

cc @github/docs-engineering @jeffmcaffer

@zeke zeke added the engineering Will involve Docs Engineering label Oct 8, 2020
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Oct 8, 2020
@chiedo chiedo removed the triage Do not begin working on this issue until triaged by the team label Oct 14, 2020
@github-actions
Copy link
Contributor

This issue is stale because it has been open 60 days with no activity.

@JamesMGreene
Copy link
Member

For the short term, we've also clarified this in the contributing/development documentation: 054f9ab

@chiedo chiedo added good first issue Good for newcomers help wanted Anyone is welcome to open a pull request to fix this issue labels Feb 23, 2021
@chiedo chiedo added feature and removed feature labels Mar 18, 2021
@janiceilene janiceilene added the needs content strategy Wait to open a PR until there's an approved content strategy in the issue label Jun 18, 2021
@FarazzShaikh
Copy link

@zeke Why not use the postinstall script in the package.json to run the build script. It only runs once after npm install.

cc: @JamesMGreene @janiceilene

@janiceilene janiceilene removed the needs content strategy Wait to open a PR until there's an approved content strategy in the issue label Jun 22, 2021
@zeke
Copy link
Contributor Author

zeke commented Jun 25, 2021

Good idea @FarazzShaikh! That might work. Would you care to give it a try and open a PR if it does?

@FarazzShaikh
Copy link

Will do later today! Could you assign this issue to me so it doesn’t slip my mind? Thank you!

@FarazzShaikh
Copy link

This does indeed work. Adding the script "postinstall": "npm run build" to package.json does build once after installations. However, one test (tests/routing/developer-site-redirects.js) fails when running npm run test but i don't think it's related to this.

Also, you might have to change your Actions workflows as npm ci also calls postinstall thus making the npm run build commands redundant.

@zeke if you're fine with this, I can make the changes to the workflows and the package.json then PR it in.

@JamesMGreene
Copy link
Member

JamesMGreene commented Jun 25, 2021

@FarazzShaikh We're definitely open to that! FWIW, your PR will receive a message saying we don't accept contributions to workflow files but we're willing to make exceptions for small change sets like what you mentoned. 👍🏻

One suggestion, though, would be to use the prepare script instead of postinstall as that more closely aligns with our needs (building/transpiling code) semantically.

@FarazzShaikh
Copy link

Sounds good, I’ll make the required changes with prepare And PR it in

@JamesMGreene
Copy link
Member

JamesMGreene commented Jul 29, 2021

We've decided not to implement this behavior for the time being.

More details: #7713 (comment)

jnidzwetzki pushed a commit to jnidzwetzki/docs that referenced this issue Oct 6, 2022
* New issues from Moz, plus some general tidy-up

* Issue github#222

* Issue github#215 & remove unused links

* Others from Moz

* Moar 404s, syntax, etc.

* make fixing changes to links

Co-authored-by: Miranda Auhl <miranda@timescale.com>
Co-authored-by: Ryan Booz <ryan@timescale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Will involve Docs Engineering good first issue Good for newcomers help wanted Anyone is welcome to open a pull request to fix this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@zeke @JamesMGreene @chiedo @janiceilene @FarazzShaikh and others