fix(typescript)!: don't resolve filtered files (#1267)

[tk-1io]

Rollup Plugin Name: typescript

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:
#1267

Description

BREAKING CHANGES: The signature of the error being thrown has changed.

This should fix the issue #1267, which is caused by the use of order: "post" as part of #1245. It will prevent the typescript plugin from resolving files which are not included by its provided configuration/options. Instead the node-resolve plugin will again handle these cases, which is able to correctly handle the browser config parameter. The same fix has been done for the rollup-plugin-typescrip2 here: ezolenko/rollup-plugin-typescript2@f6db596

[shellscape]

This is actually a breaking change, because the error being thrown has changed signature for that scenario. I'd also like to see a test with a fixture containing the browser setting so that any possible future regressions guard against edge cases with that as well.

[shellscape]

@lukastaegert getting some new CI errors. could they be ralated to your recent work?

[tk-1io]

This is actually a breaking change, because the error being thrown has changed signature for that scenario. I'd also like to see a test with a fixture containing the browser setting so that any possible future regressions guard against edge cases with that as well.

Yes, you are correct about the changed error signature (though, rollup-plugin-typescript2 released the same fix as a patch, I think this is probably because the plugin was never supposed to resolve any filtered paths in the first place).

Regarding the test, I'm not sure how I should cover the browser setting in a typescript-plugin test, because typescript does not seem to care about this setting at all when resolving paths (but the node-resolve plugin does). This means, even with this fix, the browser setting problem can still occur when someone configures the typescript plugin or his tsconfig.json in a way so it won't exclude imports to other packages (i.e. node_modules/...). To completely get rid of the browser setting problem, typescript itself would need to be able to handle this setting.
The point here is that, with a typical typescript setup, only the projects *.(m|c)ts files are meant to get compiled, while the files of dependencies inside node_modules are excluded from this process, hence the typescript plugin should also not be the one who is responsible to resolve the paths into these dependencies. Instead the node-resolve plugin is here to do this.
So in a nutshell, the problem arose because the typescript plugin tries to resolve a path which it is not supposed to do, hence "stealing" the resolve-job from the node-resolve plugin (which would correctly respect the browser setting). The problem was not apparent before, because in a typical rollup configuration the plugins are ordered in a way so that the node-resolve plugin gets executed before the typescript plugin, therefore prevented the typescript plugin from executing its flawed resolve behaviour, but this changed due to the mentioned pull #1245. This was also the reason why I just updated the existing test, as I think it addresses exactly this case: checking that the typescript plugin doesn't do any work on files that aren't its business. It now just exits a step earlier (when trying to resolve, instead of afterward trying to parse it).

Regarding the CI problems: I could not run the tests without some changes (e.g. packages/typescript/ava.config.js symlink was broken due to a renamed file), so I too think the recent changes are the ones to blame here.

[lukastaegert]

Sorry about the CI issues, if you rebase against latest master and run pnpm i, everything should be working again. I was working on a batch update of all plugins that was taking a little longer than I expected and you happened to stumble into the middle of it, sorry for that.

[tk-1io]

Thanks Lukas, after the rebase everything worked fine again.

@shellscape Based on my previous comment, do you have any suggestions on how we can proceed with this issue?

[shellscape]

My only thought is to brute force a unit test of that line, where that condition causes the error that was changed. You don't necessarily need to have TS recognize browser settings as long as you can get that line of code to execute and fail. This is one of those changes that we need regression protection against to be sure.

[shellscape]

Looks like CI cleaned itself up

[shellscape]

Thanks for being patient on this one. I'd like one more maintainer to put eyes on this before we merge.