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

Improve webpack build time #5132

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Improve webpack build time #5132

merged 2 commits into from
Nov 13, 2023

Conversation

bmesuere
Copy link
Member

This pull request tweaks the webpack config to improve the build time. This will speed up the github actions.

There are 2 changes:

  • Don't minimize the code (using terser) when building for tests
  • Don't typecheck when building the code, this is checked in the linting action

Build times on my macbook:

Version Time in s
Original version 34.06
Don't minimize while testing 16.56
Don't typecheck 12.29

@bmesuere bmesuere added the chore Repository/build/dependency maintenance label Nov 12, 2023
@bmesuere bmesuere requested a review from a team as a code owner November 12, 2023 16:33
@bmesuere bmesuere requested review from niknetniko, chvp and jorg-vr and removed request for a team November 12, 2023 16:33
@bmesuere
Copy link
Member Author

bmesuere commented Nov 12, 2023

We are getting 2 new warnings (which is strange since we're doing less):

WARNING in ./app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts 1229:80-95
export 'SavedAnnotation' (imported as 'SavedAnnotation') was not found in 'state/SavedAnnotations' (possible exports: savedAnnotationState)
 @ ./app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts 1597:0-33
 @ ./app/javascript/packs/saved_annotation.js

WARNING in ./app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts 1229:146-161
export 'SavedAnnotation' (imported as 'SavedAnnotation') was not found in 'state/SavedAnnotations' (possible exports: savedAnnotationState)
 @ ./app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts 1597:0-33
 @ ./app/javascript/packs/saved_annotation.js

@bmesuere
Copy link
Member Author

The warning seems to be normal behaviour according to the ts-loader docs.

If you enable this option, webpack 4 will give you "export not found" warnings any time you re-export a type:

WARNING in ./src/bar.ts
1:0-34 "export 'IFoo' was not found in './foo'
 @ ./src/bar.ts
 @ ./src/index.ts

The reason this happens is that when typescript doesn't do a full type check, it does not have enough information to determine whether an imported name is a type or not, so when the name is then exported, typescript has no choice but to emit the export. Fortunately, the extraneous export should not be harmful, so you can just suppress these warnings:

Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

Do we want to disable the warning(s) mentioned?

@bmesuere
Copy link
Member Author

They are just warnings, so I would not disable them since the recommended way to do is is a regex which might also match real errors?

Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

does this mean that we can move the types back to devdependencies?

@bmesuere
Copy link
Member Author

Possibly indeed, but this needs to be tested.

@jorg-vr
Copy link
Contributor

jorg-vr commented Nov 13, 2023

Naos also runs production deploy, sit it can be tested there (or by only installing production deps locally)
Will you tests this in this pr or should I do it in a future pr?

@bmesuere
Copy link
Member Author

bmesuere commented Nov 13, 2023

I would do it in a future PR because this one has no impact on production whereas moving dependencies might have.

There might also be some other optimizations possible. I noticed that we also include terser in the rubygems file. This might mean that terser is also run as part of sprockets.

@bmesuere bmesuere merged commit 622f35e into main Nov 13, 2023
15 checks passed
@bmesuere bmesuere deleted the chore/speed-up-webpack branch November 13, 2023 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository/build/dependency maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants