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 skip resolving files imported by other plugins #365

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 23, 2022

Summary

Resolve files imported from non-TS files / other plugins / JS files, such as TS files imported by Svelte files

Details

  • previously the allImportedFiles Set was basically used to skip any files that were imported by other plugins

    • it only filtered out on importer (not importee)
    • this is a bit of a problem though, as other plugins that create JS code, or allowJs JS code, or heck, even regular JS code that Rollup processes without plugins, may actually import TS files
      • and Rollup's plugin system is designed to handle that scenario, so rpt2 actually should resolve those files
        • notably, rpt2 already transforms those files, it just didn't resolve them
          • which is why using @rollup/plugin-node-resolve with a .ts extension solved this as a workaround
            • the file would be resolved by node-resolve then actually transformed by rpt2
            • but rpt2 should resolve TS files itself, node-resolve shouldn't be necessary
      • this caused the issue that extensionless imports from such files wouldn't get resolved to TS files and may cause Rollup to error
  • resolveId is actually the last remaining place where allImportedFiles was used

    • so after removing it from here, we can just remove it entirely
      • which should be a small optimization
  • for reference, allImportedFiles would be any included files as well as TS files that have been transformed (at this point in the build) and their references

    • this is a pretty gigantic list as is, so I'm actually not sure that removing this is actually that big of a de-opt
      • and it only affects mixed TS / non-TS codebases too

Historical Context

- previously the `allImportedFiles` Set was _basically_ used to skip any files that were imported by other plugins
  - it only filtered out on `importer` (not `importee`)
  - this is a bit of a problem though, as other plugins that create JS code, or `allowJs` JS code, or heck, even regular JS code that Rollup processes without plugins, may actually import TS files
    - and Rollup's plugin system is designed to handle that scenario, so rpt2 actually _should_ resolve those files
      - notably, rpt2 already _transforms_ those files, it just didn't resolve them
        - which is why using `@rollup/plugin-node-resolve` with a `.ts` extension solved this as a workaround
          - the file would be resolved by `node-resolve` then actually transformed by rpt2
          - but rpt2 should resolve TS files itself, `node-resolve` shouldn't be necessary
    - this caused the issue that extensionless imports from such files wouldn't get resolved to TS files and may cause Rollup to error
      - in particular, this popped up with extensionless imports of TS files from Svelte files

- `resolveId` is actually the last remaining place where `allImportedFiles` was used
  - so after removing it from here, we can just remove it entirely
    - which should be a small optimization

- for reference:
  - `allImportedFiles` would be any `include`d files as well as TS files that have been transformed (at this point in the build) and their references
    - this is a pretty gigantic list as is, so I'm actually not sure that removing this is actually that big of a de-opt
      - and it only affects mixed TS / non-TS codebases too
  - this seems to have been added in b0a0ecb, but that commit doesn't actually seem to fix the issue it references
    - see my root cause analyses in the issues
@agilgur5 agilgur5 added 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 labels Jun 23, 2022
@ezolenko ezolenko merged commit b0e3922 into ezolenko:master Jun 24, 2022
@agilgur5 agilgur5 deleted the fix-resolve-all-files branch July 2, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollup error Could not resolve TS files without extension (.ts) from Svelte files
2 participants