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

Use --build #487

Merged
merged 3 commits into from
May 19, 2021
Merged

Use --build #487

merged 3 commits into from
May 19, 2021

Conversation

RA80533
Copy link
Contributor

@RA80533 RA80533 commented May 8, 2021

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

Before

$ time npm run build

> codeql@0.0.0 build
> tsc


real    0m7.986s
user    0m11.848s
sys     0m0.684s

After

$ time npm run build

> codeql@0.0.0 build
> tsc --build


real    0m0.557s
user    0m0.427s
sys     0m0.131s

The change results in ~1,300% faster builds.

Closes #486

@RA80533 RA80533 closed this May 8, 2021
@RA80533 RA80533 reopened this May 8, 2021
@robertbrignull
Copy link
Contributor

Thanks for pointing this out. I didn't actually know this existed and behaved this way. Sadly I don't think this is quite such a magic bullet as it seems, but it's still a strict benefit and we should do it.

The thing that's missed from the timing data is that tsc --build is only that fast when there's nothing to do. If you run tsc --build repeatedly then it'll take no time at all, whereas running just tsc takes 11s each time. However if you delete the lib directory or change any .ts file then tsc --build and tsc both take the same time.

Generally you're going to run this command when you've changed something, so I don't think this is going to make things faster in the common case, but it is still better in the case where there's nothing to do so we should still make this change.

@robertbrignull
Copy link
Contributor

@RA80533, the CI is complaining that the node modules are out of date, but what it really is I think is that trailing newline in package.json. Can you run npm ci && npm run removeNPMAbsolutePaths to let it format this how it wants. We use this check to make sure the actual node_modules directory is kept up to date, but unfortunately it also sometimes fails on trivial things like this.

@RA80533
Copy link
Contributor Author

RA80533 commented May 10, 2021

Your observation is correct and I should’ve been more specific in my metrics. Builds during development should ultimately end up being much faster, with the ~1,300% increase being the ceiling when no changes have been made. Enabling incremental may also be of additional use but I can’t recall if --build does so implicitly.

I’ll run those commands on the branch when I get home.

@RA80533
Copy link
Contributor Author

RA80533 commented May 11, 2021

It appears that npm ci creates a huge diff. None of the package.json files in node_modules have a final newline. Is this something that should be done separate from this PR?

@aeisenberg
Copy link
Contributor

Did you also run npm run removeNPMAbsolutePaths? If you just run npm ci, then you will see a large diff. The second command will remove all the diffs.

@aeisenberg aeisenberg merged commit bc39b21 into github:main May 19, 2021
@aeisenberg
Copy link
Contributor

Thanks for sticking with this change!

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.

Transpile with --build
3 participants