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

Rollback #208: Reinstate rollup-plugin-typescript2 #287

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Rollback #208: Reinstate rollup-plugin-typescript2 #287

merged 1 commit into from
Oct 29, 2019

Conversation

medelman17
Copy link
Contributor

As described in #200, tsdx previously utilized rollup-plugin-typescript2 to compile and check TypeScript files. On account of the fact that the above plugin cannot be used alongside other rollup plugins that contain async/await syntax without a workaround, tsdx migrated to @wessberg/rollup-plugin-ts in #208. However, as discussed in #268, #274, #285 and elsewhere, the above migration caused untoward consequences in certain instances for existing tsdx users with custom babel setups.

This PR reverts support @wessberg/rollup-plugin-ts and restores rollup-plugin-typescript2 as compiler in chief.

Closes #268, #274, #285

@jaredpalmer jaredpalmer merged commit 3989277 into jaredpalmer:master Oct 29, 2019
@markerikson
Copy link
Collaborator

Hmm. We specifically updated Redux Starter Kit to use 0.10.6 because it started correctly merging all our declarations into a single index.d.ts file, vs a separate .d.ts file per source file. Does this rollback still do things as a single index.d.ts file?

@jaredpalmer
Copy link
Owner

jaredpalmer commented Oct 29, 2019

Yes, I believe so. The issue was that the rollup-plugin-ts really did not work as advertised and introduced more problematic and inconsistent bugs such as duplicative references directives and incorrect declarations. @markerikson why does merging declarations matter outside of aesthetics?

@markerikson
Copy link
Collaborator

I know @phryneas pointed out some issues where that might not work with certain Webpack setups, apparently. See reduxjs/redux-toolkit#212 (comment) for discussion.

@jaredpalmer
Copy link
Owner

jaredpalmer commented Oct 29, 2019

@medelman17 @markerikson FWIW, my OG formik rollup setup (pre-tsdx) didn't use rollup-plugin-typescript2 at all, instead it did a 2 pass compile. First pass used tsc-watch (which is just tsc with an onSuccess flag) to compile typescript to a temp directory. Second pass just used rollup to bundle that JS up and move the types to the dist dir. The clear benefit of this setup is that you can just use good ol' tsc and let it do it's thing. Then you can let people go buck wild with rollup. The downside of course it that you have 2 processes (even when run concurrently)

@z0al
Copy link

z0al commented Oct 29, 2019

Does this rollback still do things as a single index.d.ts file?

No. At least for me.

Closes #268, #274, #285

FYI @medelman17 close block doesn't accept multiple parameters, the correct syntax is:

Closes #268, Closes #274, Closes #285

Thought you might find this useful :)

@wessberg
Copy link

wessberg commented Oct 29, 2019

Yes, I believe so. The issue was that the rollup-plugin-ts2 really did not work as advertised and introduced more problematic and inconsistent bugs such as duplicative references directives and incorrect declarations.

Merging declarations (and splitting them across rollup chunks, rewriting and tree-shaking them), something that rollup-plugin-typescript2 doesnt do and even tsc itself doesn't do, is non-trivial. I think it's a bit harsh to right out say it is not working as advertised 🙂. I'm putting a lot of effort into fixing whatever issues that remain with declaration bundling and am addressing the issues one at a time.

Currently I'm addressing a lot of the recently discovered issues through this repo on a separate branch. It has been very helpful to get it into more developers hands so I can discover whatever bugs there are.

@jaredpalmer
Copy link
Owner

Didn't mean to come off as rude, nor does this have anything to do with lack of effort. I understand that you're working through bugs, but until those are fixed, we need to ensure that the published version of tsdx still works in the interim. Looking forward to when the declaration merging is stuff is sorted out. It's very cool and exciting.

@tunnckoCore
Copy link

tunnckoCore commented Nov 1, 2019

I was betting it will conflict. 😆 The customBabelRollup plugin should be removed (entirely dropping rollup-plugin-babel) and passed as babelConfig to the ts plugin, I believe.

But yea, I agree that we need to wait :) Need experimenting, definitely.

@markerikson
Copy link
Collaborator

@jaredpalmer : no, it looks like v0.11.0 is not combining types into a single index.d.ts file.

I just tried updating Redux Starter Kit and rebuilding, and it's back to individual .d.ts files per source file.

@smashercosmo
Copy link

hm... so, what's the recommended approach then to introduce additional rollup plugins that contain async/await?

@agilgur5 agilgur5 added the topic: v0.10.x Issues related to broken builds in v0.10.x / @wessberg/rollup-plugin-ts label Mar 16, 2020
@agilgur5 agilgur5 added the topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2 label Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2 topic: v0.10.x Issues related to broken builds in v0.10.x / @wessberg/rollup-plugin-ts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No .d.ts created after update to 0.10.0
8 participants