Combine build tasks

[nyandika]

Changes proposed

• Add an npm task to run lint, prettier, typecheck and build combined
  Added it to github action, and renamed it. Should now automatically run for all PRs and merges
• As optional: pin dependencies to specific versions

Check List (Check all the applicable boxes)

• My code follows the code style of this project.
• My change requires changes to the documentation.
• I have updated the documentation accordingly.
• All new and existing tests passed.
• This PR does not contain plagiarized content.
• The title of my pull request is a short description of the requested changes.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
reactdevske-website	✅ Ready (Inspect)	Visit Preview	Oct 10, 2022 at 4:06PM (UTC)

[antosan]

compile is synonymous with transforming code, but that is not what we are doing here. I suggest the name validate for this script (we run this script to validate the status of the project)

Also, the way the commands are currently chained makes it hard to keep track of the different outputs. If one process fails, others keep running which is unnecessary since at this point the validation has already failed. I am also not sure if the && works as expected on windows.

For this script, I suggest using the concurrently package to run the multiple commands concurrently. We can add it to the project using npm i -D concurrently and it works cross-platform (including Windows), produces output that is easy to follow with prefixes and will allow us to kill all commands if one fails.

Then the validate script will look like this:

"validate": "concurrently --kill-others-on-fail -g -p \"[{name}]\" -n \"lint,typecheck,build\" \"npm:lint -s\" \"npm:typecheck -s\" \"npm:build -s -- --no-lint\""

I do not suggest running prettier in the validation since it has the possibility of trying to modify the source code in CI.

Why these options:

• --kill-others-on-fail - all commands are killed if one fails
• -g - even though the commands are run concurrently, we order the output as if they were run sequentially
• -p - we use the names we assign to the commands as prefixes on the terminal output
• -n - these are the names that will be used in the prefixes
• -s - we tell npm to suppress the output on the terminal when running the commands, this is cleaner
• --no-lint - we can skip linting a second time since we just ran the lint script (all for speed)

The output would look like this:

[lint] ✔ No ESLint warnings or errors
[lint] npm run lint -s exited with code 0
[typecheck] npm run typecheck -s exited with code 0
[build] warn  - Linting is disabled
[build] info  - Checking validity of types...
[build] info  - Creating an optimized production build...
[build] info  - Compiled successfully
[build] info  - Collecting page data...
[build] info  - Generating static pages (0/9)
[build] info  - Generating static pages (2/9)
[build] info  - Generating static pages (4/9)
[build] info  - Generating static pages (6/9)
[build] info  - Generating static pages (9/9)
[build] info  - Finalizing page optimization...
[build]
[build] Route (pages)                              Size     First Load JS
[build] ┌ ○ /                                      841 B          84.3 kB
[build] ├   /_app                                  0 B            83.4 kB
[build] ├ ○ /404                                   194 B          83.6 kB
[build] ├ ○ /about                                 439 B          83.9 kB
[build] ├ λ /api/hello                             0 B            83.4 kB
[build] ├ ○ /contact                               439 B          83.9 kB
[build] ├ ● /events                                1.07 kB        84.5 kB
[build] ├ ○ /forum                                 438 B          83.9 kB
[build] ├ ○ /members                               444 B          83.9 kB
[build] └ ○ /news                                  437 B          83.9 kB
[build] + First Load JS shared by all              86.1 kB
[build]   ├ chunks/framework-a87821de553db91d.js   45 kB
[build]   ├ chunks/main-a5af4d1b6edd23b1.js        30.9 kB
[build]   ├ chunks/pages/_app-7103b2b073fdc724.js  6.74 kB
[build]   ├ chunks/webpack-69bfa6990bb9e155.js     769 B
[build]   └ css/61d2eb34b00a4989.css               2.67 kB
[build]
[build] λ  (Server)  server-side renders at runtime (uses getInitialProps or getServerSideProps)
[build] ○  (Static)  automatically rendered as static HTML (uses no initial props)
[build] ●  (SSG)     automatically generated as static HTML + JSON (uses getStaticProps)
[build]
[build] npm run build -s -- --no-lint exited with code 0

[nyandika]

I did ponder for a bit what to call the script and my two brain cells weren't sparking in rhythm to give me a good enough name. was leaning towards build but that is already in use.

Choosing to put all the subtasks in the same file is generally a personal preference coming from a background of using gulp.js to run sub-tasks

I did contemplate using concurrently but debated need to add another lib. If it's okay to add one then I can refactor to add it in. Also going down this path then have we considered using task runners to do these build and validate steps?
sidenote: && does work in windows, I am on my windows laptop at the moment

For the prettier part: Running it locally has the potential to alter code; but again, is that not the point of having it in the first place. as for the CI part it won't persist code changes because whatever changes it makes, should it make them, are not being committed as part of the validate process. Ideally a dev should run the scripts locally before pushing changes but should they skip this the CI should catch these and report the appropriate failure

I'll wait for your response before refactoring in the necessary changes and sending them out for review

[antosan]

I did contemplate using concurrently but debated need to add another lib. If it's okay to add one then I can refactor to add it in.

We will add this as a dev dependency for better DX, and it should not affect the production bundle. If you have already contemplated using it and given the benefits I mentioned, I would say please refactor and add it in.

Also going down this path then have we considered using task runners to do these build and validate steps?

Well, npm is also a task runner and these tasks are simple enough and perfectly suited for npm.

sidenote: && does work in windows, I am on my windows laptop at the moment

Wow, that's great. Windows seems to have improved a lot since the last time I used it for development.

For the prettier part: Running it locally has the potential to alter code

Yes, that's my point. The prettier script is handy and devs should run it locally. The issue I raised is that the current prettier script we have uses the --write flag, which instead of failing with an error, would instead rewrite all the processed files in place and succeed and this will not break the CI pipeline. It will be like running eslint with the --fix flag in CI. The developer would have to go and manually check the CI output every time to know what happened which beats the logic. I agree that it would be nice for the CI to catch these, but for that, we need a separate CI script. I suggest adding a script that we shall run in CI that uses the --check flag (instead of --write). This will cause it to return an exit code of 1 if something isn't formatted correctly and this would then break the CI. Then the current script will be used locally to format the files. We can then include this prettier:check command in concurrently.

"prettier:check": "prettier --check --ignore-path .gitignore .",
"prettier:format": "prettier --write --ignore-path .gitignore .",

[nyandika]

Agreed.
I have pushed a new commit

[antosan]

What is the reason for pinning ALL the dependencies to specific versions?

If it is to protect against breakage, then this rarely works since all the sub-dependencies are still free to update. Also chances that a sub-dependency breaks the app increases with pinning. I think the lock file already does a good job of locking the packages we depend on including sub-dependencies. The lock file gives us "pinning" even without removing the caret.

Regarding removing the caret, I am not sure how maintainable this is. This is a public repo and the default npm behaviour is to use the caret when updating package.json unless the default is changed which a majority of developers do not change. It will be a lot of manual work to maintain this as a standard, yet the benefit is not yet clear to me. (For specific packages with special use cases, it is okay to remove the caret and update the lock file, what I am struggling with is why we should do it for all the packages in the project)

[nyandika]

I had assumed it is good practice to pin dependencies. This again coming from my background experiences.
This article touches on this.

As for updating dependencies it doesn't have to be a manual process. We have bots like Renovate that can handle that and with specified lengths of time to do so

I will go ahead and revert this and hold onto it to push with the other changes above

[antosan]

I had assumed it is good practice to pin dependencies. This again coming from my background experiences.
This article touches on this.

I read the article, thanks for sharing, and even the author agrees that this is controversial. From my personal experience having a lock file (which we already do) is more than enough to mitigate the problems mentioned. I am open to learning more about your experiences as well. Can you open a new issue (or better a discussion) in the repo where we can discuss this and make a decision with other developers' thoughts also included?

As for updating dependencies it doesn't have to be a manual process. We have bots like Renovate that can handle that and with specified lengths of time to do so

I meant how to make sure that whenever a new developer (or a bot like renovate) adds a new package, the caret is not automatically added back in. I know we simply need to add a .npmrc file in the project, but let's push the other changes and discuss this separately. Thanks.

[nyandika]

I will create a discussion item on the same

[antosan]

LGTM 👍

Thanks @nyandika