-
Notifications
You must be signed in to change notification settings - Fork 334
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
Skip JSDoc, SassDoc, Rollup stats during dev, add ESLint #4241
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for f655b6d |
I don't have any major problem with this - takes my rebuild time from 4+ seconds to 1-2 seconds. One slight concern is it makes it more annoying to develop things that focus on the stuff that's skipped. I can't just run Relatedly, does this mean we'll get all the type complaints only when we run the Github action workflow? |
@domoscargin Yeah, that's the downside for documentation and stats For others reading this you can still run them manually at any time: npm run build:jsdoc --workspace @govuk-frontend/review
npm run build:sassdoc --workspace @govuk-frontend/review
npm run build:stats --workspace govuk-frontend
Yeah but up for discussion? Similar with Prettier, we don't run it via Good to find the right balance |
Sometimes you might want to skip the slow npm run dev --workspace govuk-frontend
npm run dev --workspace @govuk-frontend/review Another top tip |
Yeah, I think running the doc stuff manually will be absolutely fine 99.5% of the time so that's grand. Completely on the fence about whether it's nicer to develop without getting type failures, or whether it'd be worse to get a bunch of 'em at once and potentially have to do a bigger refactor. Hopefully somebody else chimes in and leans one way or the other! |
@domoscargin I'd quite like them turned on too, but it's usually just me with that opinion 😆 If you don't mind TypeScript We do this in GitHub Actions already:
Type check
time npm run build:types --workspace govuk-frontend Type check with cache
time npm run build:types --workspace govuk-frontend -- --incremental |
1e93a54
to
f81672b
Compare
18267fc
to
468fdf4
Compare
@domoscargin I've added the TypeScript compiler Ready for review again |
I think this makes sense most of the time but that there are scenarios where it'd be useful to see the stats or docs update as you make changes. How hard would it be to:
That way you won't end up in situation where you can forget you're looking at stale stats / docs. Thoughts? |
f81672b
to
b83a894
Compare
Runs before development sessions but not during
Runs before development sessions but not during
Runs before development sessions but not during
468fdf4
to
26acda7
Compare
@36degrees Sounds like a nice addition, can decide the best approach today? In readiness, I've pushed |
26acda7
to
1b8cae7
Compare
1b8cae7
to
011b769
Compare
Since we no longer build JSDoc, SassDoc or stats via Gulp we can turn these back on
Whilst it’s important to re-compile the Review app when the package changes, we can avoid running StyleLint unless Review app files have changes Also makes paths consistent since we had a mixture of template strings, path joins, and duplicated some ignore paths already found in config files
We already have type checks in editors and IDEs, plus we run `lint:types` during the GitHub Actions workflow So this check is more useful as a watch task for early feedback Adds flag `--incremental` to use the compiler cache (10x faster) and flag `--pretty` to preserve colour output via `concurrently` CLI
The Rollup stats and webpack example don’t run Gulp so this gives us a consistent way to “reset everything” before a build
011b769
to
f655b6d
Compare
We talked about this in today's dev catch-up and will keep the speed improvements Suggested flags to "opt in" to JSDoc, SassDoc or Rollup stats can be added in future In the meantime we can use existing npm scripts when optional features are required: npm run build:jsdoc --workspace @govuk-frontend/review
npm run build:sassdoc --workspace @govuk-frontend/review
npm run build:stats --workspace govuk-frontend These will automatically appear in the Review app when available See PR description for updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Can definitely feel the difference when developing locally, and there's not as much gumph in my terminal to scroll through to find any relevant errors.
And the type checking seems really speedy too!
Skip JSDoc, SassDoc, Rollup stats during dev, add ESLint
This PR does a few things to speed up development:
build:types
to a watch tasknpm run clean
task for workspaces without GulpWhere have my type checks gone?
Type checks moved to
npm run lint
last month so we can skipnpm run build:types
In development, ESLint, StyleLint and the TypeScript compiler now run via
gulp.watch()
insteadWhere have my stats gone?
JSDoc, SassDoc and Rollup stats are now considered optional in development
That said, the Review app will turn on these optional sections when they're built using: