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 resolve filtered files #428

Merged
merged 3 commits into from
Oct 3, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Oct 3, 2022

Summary

Early return after filter in the resolveId hook, same as we do in transform

Details

  • if they're excluded / not included, then we shouldn't be processing them

    • we're already not transforming them, so this just applies the same exclusion to resolving
    • this is partly a regression from fix: don't skip resolving files imported by other plugins #365, as that removed the allImportedFiles Set that previously filtered out files not in the tsconfig include
      • but that itself was a regression that was removed -- files that didn't pass filter should have never been resolved
        • basically, the allImportedFiles regression was covering up this long-standing bug
  • also move .d.ts check to above the filter check

    • we shouldn't be adding declarations to the cache, in particular as we don't process declarations, so they'll never be marked as dirty
    • having this check above the filter should be slightly more efficient as well (as would not having these files in the cache graph)

types: be more specific with filter's type

  • no need for this to be any

Review Notes

I also did a build and version bump in this PR as folks have been waiting for a patch for a few weeks, c.f. #422 (comment) (I also recommended a patch in that PR's opening comment)

- if they're `exclude`d / not `include`d, then we shouldn't be processing them
  - we're already not transforming them, so this just applies the same exclusion to resolving
  - this is _partly_ a regression from b0e3922, as that removed the `allImportedFiles` Set that previously filtered out files not in the `tsconfig` `include`
    - but that _itself_ was a regression that was removed -- files that didn't pass `filter` should have _never_ been resolved
      - basically, the `allImportedFiles` regression was covering up this long-standing bug

- also move `.d.ts` check to above the `filter` check
  - we shouldn't be adding declarations to the `cache`, in particular as we don't process declarations, so they'll never be marked as dirty
  - having this check above the `filter` should be slighltly more efficient as well (as would not having these files in the cache graph)

types: be more specific with `filter`'s type
- no need for this to be `any`
- patch release has been waiting for a few weeks
- patch bump with the past few fixes

- bump internal rpt2 version to 0.34.0
@agilgur5 agilgur5 added kind: bug Something isn't working properly scope: integration Related to an integration, not necessarily to core (but could influence core) kind: regression Specific type of bug -- past behavior that worked is now broken version: patch Increment the patch version when merged labels Oct 3, 2022
@agilgur5 agilgur5 requested a review from ezolenko October 3, 2022 00:38
@ezolenko ezolenko merged commit f6db596 into ezolenko:master Oct 3, 2022
@ezolenko
Copy link
Owner

ezolenko commented Oct 3, 2022

@agilgur5 released in 0.34.1, thanks

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Oct 4, 2022

updated the release notes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly kind: regression Specific type of bug -- past behavior that worked is now broken scope: integration Related to an integration, not necessarily to core (but could influence core) version: patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

package.json browser field has no effect (bundling Axios)
2 participants