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

properly support watch mode when transforming .ts files #249

Merged
merged 7 commits into from
Feb 13, 2023
Merged

properly support watch mode when transforming .ts files #249

merged 7 commits into from
Feb 13, 2023

Conversation

rayzr522
Copy link
Contributor

closes #248

while the diff looks noisy, these changes preserve the codepaths 1:1 while making the actual logic involved much more digestible

the main big change is in resolving modules, where we now resolve both the code as well as the typescript-compiler-specific source & program values. this makes the logic inside of transform() way simpler, where we can just check that resolveModule actually returned something, and if so we know we can pass the code straight along to the inner transform plugin

I also did my best to remove a lot of mutability in variables as it was making it hard to follow the logic, as well as make the types a little stricter/more explicit

if this PR is too heavy-handed, I am happy to restore some of the original structure (e.x. re-inlining getModule so we don't need to pass around a context object) while preserving the main core changes that enabled better watch mode support. however, as you mentioned in #248 that this repo is mostly in maintenance mode, I am hoping that these changes will be welcome as I feel they greatly improve the readability & robustness of the code :3

all tests pass :)

Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks very good!

@Swatinem Swatinem merged commit de2f8b1 into Swatinem:master Feb 13, 2023
@rayzr522 rayzr522 deleted the officially-support-watch-mode branch February 13, 2023 19:00
@Swatinem
Copy link
Owner

I released this as 5.2.0 as well.

Interesting that dogfooding this for the plugins own type definitions, the changes to use satisfies Plugin rather than plugin: PluginImpl<...> means that it will now inline the whole definition of the output type, instead of using a oneliner for the plugins type definition.

TBH this is the first time I saw satisfies in action (like I said, I’m not doing active TS development for years), but it looks like using an explicit type declaration was better than having the return type inferred.

@rayzr522
Copy link
Contributor Author

rayzr522 commented Feb 13, 2023

the changes to use satisfies were mainly so that we know the exact hooks/functions in each plugin, eliminating the need to handle the nullability of the transform plugin's hooks since typescript now knew that the functions we required were 100% present

I'm all for exposing the main plugin with an explicit typedef if you don't want to have the public side of things to have those inferred typings, but at least for the inner transform plugin, it was hugely helpful to be able to have more explicit typings that mirrored the actual code

tysm for getting this released!

mrm007 pushed a commit to mrm007/rollup-plugin-dts that referenced this pull request May 1, 2023
cschramm added a commit to cschramm/rollup-plugin-dts that referenced this pull request Sep 15, 2023
The `generateDtsFromTs` path expects cases where `!module.source` but since Swatinem#249 `getModule` does not actually return it as it fails trying to use it for gathering `code`.
@cschramm cschramm mentioned this pull request Sep 15, 2023
Swatinem pushed a commit that referenced this pull request Sep 15, 2023
The `generateDtsFromTs` path expects cases where `!module.source` but since #249 `getModule` does not actually return it as it fails trying to use it for gathering `code`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

watch mode does not watch for changes to files when using indirectly inferred types
2 participants