Skip to content

Conversation

@jesusoterogomez
Copy link
Contributor

@jesusoterogomez jesusoterogomez commented Nov 4, 2025

Linting currently depends on multiple tasks:

  • one of them is validate-links(): A JS script that needs to be built in the support/dist directory before being able to run ./lint locally

This means, if ./lint is run before building the program, it fails due to a missing script, and contributors upon a clean install, would have to build the program at least once (npm run build) to be able to run ./lint

Otherwise, they'd get this error:
Error: Cannot find module '.../hyf/program/support/dist/validateLinks.js'

This PR fixes that:

This change ensures the support project is built with the local version of tsc before running the linter. Only if it doesn't exist already.

@jesusoterogomez jesusoterogomez requested a review from a team as a code owner November 4, 2025 15:16
@jesusoterogomez jesusoterogomez changed the title Linting: Ensure the validateLinks.js linting script is built Linting: Ensure the validateLinks.js script is built Nov 4, 2025
Copy link
Contributor

@shpomp shpomp left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@adamblanchard adamblanchard 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 it's a great idea to make this less of a confusing issue for new contributors, especially.

I'm thinking though, rather than coupling/duplicating the build command to this script, instead we do the same check but print a more useful error instead, like "It looks like you haven't built the project yet. Try running npm install and npm run build and try again."

That way, the build command isn't defined in more than one place, but it still helps resolve the issue more transparently. What do you think?

@jesusoterogomez
Copy link
Contributor Author

@adamblanchard,

-- sorry for the wall of text --

I initially didn't check the build command inside package.json 🙈.

I thought it was building more than just the support/tsconfig.ts, and that's why I created the specific command to build that project only. But it turns out that npm run build does precisely just that.

I've added a new commit that changes this and outputs a message:

  • The benefit is that it's not duplicated anymore (good point noticing that)

However, It's still coupled to the build script, but:

  • I don't think this is necessarily a bad thing, considering that's exactly the command the contributor would have to run next anyways.
  • If they haven't done npm install it would result in ERR_MODULE_NOT_FOUND (this applies to the rest of the linting commands as well, since they're all coming from node modules)

A longer term downside can be:

  • If the npm run build command starts to do more things, it can slow down the first instance of the linting run on clean installs. But it would be the same if it had to be run manually.

What are your thoughts? I personally think the benefits outweigh the negatives, but I'm also open to changing this to just output message and the note in the contributors README.md

@adamblanchard
Copy link
Contributor

Ah right, yeah this is also a good suggestion to use the build command directly :D

I think this is useful change, and is hardly a big commitment, so let's do it!

Thanks for explaining your thought processes so transparently - it's really nice with the contributions 🙌

@adamblanchard adamblanchard merged commit 8a81961 into HackYourFuture-CPH:main Nov 5, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants