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: filter "missed" declarations as well #347

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 5, 2022

Summary

Only output declarations for files that pass the filter check so there isn't a mismatch between JS being excluded, but DTS declarations still being output.

Details

  • these should run through the same filter as runs in transform etc
    • prior to this, the plugin exclude would filter files in transform, meaning no JS would be output for them, but would still output declarations for these very same files that no JS was produced for
      • (this would only happen if one were using an include glob or something that made the file appear twice, i.e. once by Rollup in transform and once in parsedConfig.fileNames)
    • this change makes it so the plugin exclude affects both JS and DTS output equivalently
      • it was very confusing when it didn't, and users relied on setting different tsconfig excludes to workaround this (as a tsconfig exclude would make the file not appear in parsedConfig.fileNames)

- these should run through the same `filter` as runs in `transform` etc
  - prior to this, the plugin `exclude` would filter files in
    `transform`, meaning no JS would be output for them, but would still
    output declarations for these very same files that no JS was
    produced for
    - (this would only happen if one were using an `include` glob or
      something that made the file appear twice, i.e. once by Rollup
      in `transform` and once in `parsedConfig.fileNames`)
  - this change makes it so the plugin `exclude` affects both JS
    and DTS output equivalently
    - it was very confusing when it didn't, and users relied on setting
      different `tsconfig` `exclude`s to workaround this (as a
      `tsconfig` `exclude` would make the file not appear in
      `parsedConfig.fileNames`)
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 23, 2022

Huh, so I was looking into allImportedFiles because it causes #283 (see root cause analysis in #283 (comment)), and I found that the commit that adds it also removed the filter that was already there on this line of the diff.

So this PR undoes that line. And with #176 and #346, basically that entire commit is undone....

Considering #283 is caused by the follow-up commit, it seems like the entire allImportedFiles Set may in fact be erroneous and have caused several regressions... 😕

It seems to have been created for #162 and #136 (comment) :

In any case, the main impact of allImportedFiles was undone in #176 anyway. So removing the resolution piece for #283 shouldn't have much impact, other than being a bugfix / regression fix.

@agilgur5
Copy link
Collaborator Author

Yeaaaa, did some more digging and per #283 (comment) , allImportedFiles from #162 seems to have been entirely erroneous and a source of at least 5 regressions in the past few years (some that were quite common).

The last two fixes for those regressions, this PR and #365, have yet to be released either, so like big OOF on how many regressions for how many years that's caused 😕 😕

@agilgur5 agilgur5 deleted the fix-filter-missed-declarations branch July 2, 2023 21:27
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
Projects
None yet
2 participants