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

fix: don't error out while catching a buildStart error #422

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Sep 17, 2022

Summary

If an error occurs during the buildStart hook, make sure that the buildEnd hook doesn't hide the error with its own error

Details

  • per in-line comment, if an error occurs during buildStart initialization, then the cache may not exist yet during buildDone

    • since we now use context.error instead of throw during initialization (from the options -> buildStart change), buildEnd will run during initialization - and the cache var is initialized in buildStart as well, so if an error occurs before then, the cache won't exist - we should gracefully handle this in all cases, since it's possible that even creating the cache could throw an error
  • this error was hiding the underlying error, which was problematic for DX as well as bug reports (see my issue follow-up)

  • add an integration test for tsconfig to make sure this code works

    • while this is similar to the previous tsconfig integration tests (that were moved to unit tests), this covers all cases of buildStart errors, and is not specific to the different tsconfig errors (unlike the unit tests)
    • this test will fail without the source code changes in this commit

Misc Notes

Would be good for a patch release! Especially as this error currently hides actual errors 😕

- per in-line comment, if an error occurs during `buildStart` initialization, then the `cache` may not exist yet during `buildDone`
  - since we now use `context.error` instead of `throw` during initialization (from the `options` -> `buildStart` change), `buildEnd` will run during initialization
    - and the `cache` var is initialized in `buildStart` as well, so if an error occurs before then, the `cache` won't exist
      - we should gracefully handle this in all cases, since it's possible that even creating the `cache` could throw an error

- this error was hiding the underlying error, which was problematic for DX as well as bug reports (see my issue follow-up)

- add an integration test for `tsconfig` to make sure this code works
  - while this is similar to the previous `tsconfig` integration tests (that were moved to unit tests), this covers all cases of `buildStart` errors, and is not specific to the different `tsconfig` errors (unlike the unit tests)
  - this test will fail without the source code changes in this commit
@agilgur5 agilgur5 added kind: regression Specific type of bug -- past behavior that worked is now broken scope: cache Related to the cache scope: tests Tests could be improved. Or changes that only affect tests labels Sep 17, 2022
@agilgur5 agilgur5 added the kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc label Sep 17, 2022
@ezolenko ezolenko merged commit e8f59ef into ezolenko:master Sep 20, 2022
@Eonasdan
Copy link

Hi this is currently causing issues in one of my projects. Could you push this out please?

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Oct 3, 2022

@Eonasdan this has been released in 0.34.1

@agilgur5 agilgur5 deleted the fix-buildStart-error-catch branch July 2, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc kind: regression Specific type of bug -- past behavior that worked is now broken scope: cache Related to the cache scope: tests Tests could be improved. Or changes that only affect tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done())
3 participants