Support Async/Await in Rollup Plugins by migrating to @wessberg/rollup-plugin-ts

[medelman17]

This is a WIP re #200. Sadly, I cannot, for the life of me, get the last two tests to pass. A screenshot of the issue is included below. It looks related to our babel/typescript relationship-- which is complex. I'm happy to continue digging in, but figured I would surface in case anyone could point me in the right direction.

FWIW, it looks like the new plugin has a fairly robust integration with babel. I'm not sure re purpose of the custom tsdx babel plugin, but could we accomplish the same with a .babelrc or babel.config.js file? If so, the new plugin looks capable of managing the babel stage. See Meaw.

closes #200

[tunnckoCore]

That's it.

[medelman17]

Sorry, y'all, for the delayed update, but, with one minor caveat, we should now be good to go, here. The remaining 'hitch' relates to the name of the emitted type declaration files.

In tsdx-generated package.json files we include the following configuration relevant here:

{
  ... 
  "main": "dist/index.js", 
  "typings": "dist/index.d.ts"
  ...
}

However, the plugin wants to emit declaration files on a (potential) chunk/entry-by-chunk/entry basis using actual filenames, bundle-type-suffixes, etc.. As a consequence, no index.d.ts file is ever generated. This appears to be the reason the tests above were failing--i.e., it wasn't that types/declaration files were not being included; rather, our tests were just looking for the wrong file. Maybe @wessberg can shed some additional light here, but I couldn't figure out how to configure rollup-plugin-ts to do otherwise.

Thus, to retain functional parity with respect to typings, I changed the default package.json configuration to point to the correct/equivalent file: dist/${safeName}.esm.d.ts and amended the tests accordingly. This should work, but it is not as pretty. Alternatively, we could add another build step to copy/rename the declaration file. Otherwise, I think this is baked and ready to merge.

[wessberg]

@medelman17, that's correct, rollup-plugin-ts will generate declarations for each output chunk, and the declarations will be identically named to that of the output chunks.

So if Rollup writes an output chunk to dist/index.esm.js, the declarations will be written to dist/index.esm.d.ts.

[jaredpalmer]

Nice

[timReynolds]

Happened to hit this yesterday and it's annoying to work around. Any idea when this could be merged?

[tunnckoCore]

Not especially related to the PR, but just my 2cents :)

@wessberg: So if Rollup writes an output chunk to

And that's kind of strange, but understanding it. If you output 2 formats - cjs and esm (as in my case, in different directories - dist/cjs/index.js and dist/esm/index.js), it generates basically the same type files (dist/cjs/index.d.ts and dist/esm/index.d.ts), which is unnecessary. In my case I did workaround it to just generate types at dist/types/index.d.ts. But yea, I understand why this is/should happen.

Actually, it may help for the PR. Not sure.

[medelman17]

Yes. This is precisely the hitch above. What to do about that? Since TSDX does both cjs and esm out of the box, by default we generate two identical .d.ts files (neither of which is named index.d.ts 🙄), but for the file names, using this plugin. In other words: 😬 I suppose I lean towards, ‘letting it happen,’ but I don’t have enough skills/experience to feel confident about that. That said, even writing an extra build step just to clean up the naming issue purely for aesthetics feels hack-y. So, if, as a practical matter, having the extra types included in the package has a negligible impact, I would accept that as a trade off. But I don’t know RT impact. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Charlike Mike Reagent <notifications@github.com> Sent: Monday, September 30, 2019 12:03:51 AM To: jaredpalmer/tsdx <tsdx@noreply.github.com> Cc: Michael Edelman <michael@fabulas.io>; Mention <mention@noreply.github.com> Subject: Re: [jaredpalmer/tsdx] Support Async/Await in Rollup Plugins (#208) Not especially related to the PR, but just my 2cents :) @wessberg<https://github.com/wessberg>: So if Rollup writes an output chunk to And that's kind of strange, but understanding it. If you output 2 formats - cjs and esm (as in my case, in different directories - dist/cjs/index.js and dist/esm/index.js), it generates basically the same type files (dist/cjs/index.d.ts and dist/esm/index.d.ts, which is unnecessary. In my case I did workaround it<https://github.com/tunnckoCore/configs/blob/a90e8183a4a7a678ec0e42dec253277c789a7001/packages/rollup-config/create-config.js#L219-L229> to just generate types at dist/types/index.d.ts. But yea, I understand why this is/should happen. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#208?email_source=notifications&email_token=ADQ3VLKZSNOD7MFL5CIDL6TQMF3CPA5CNFSM4IWK7QZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD74LAZY#issuecomment-536391783>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADQ3VLKLXHGCGNMY676TR3LQMF3CPANCNFSM4IWK7QZQ>.

[tunnckoCore]

Oh sure, I'm sleepy, haha. I'll check it later today: do not merge ;p

That said, even writing an extra build step just to clean up the naming issue purely for aesthetics feels hack-y

Besides that, feels too much :D

[wessberg]

@tunnckoCore, @medelman17, if you use tsc directly, the declarations will be emitted alongside the emitted JavaScript and SourceMap files, having the same basename, and this is the same default behavior of rollup-plugin-ts. It goes for both tsc and rollup-plugin-ts that you can provide a declarationDir option inside tsconfig.json to ensure that declarations are writing to a specific folder. However, there's no way of forcing a specific basename.

If this is an important feature requirement for you, I can look into providing a hook to the config that is invoked with the path to a declaration file immediately before it is emitted, which then allows you to rewrite it to something else. That would allow you to always write to the same path and with the same name, no matter the amount of Rollup outputs.

Would you be interested in this?

@tunnckoCore The generation of declarations could potentialy have some form of shared caching that may enhance performance, given that the input will be identical.

But I don't see how we could avoid generating declarations for each Rollup output, given that the output configs in Rollup can be very different. For example, the entry- and chunk names may differ.

Let me give an example:

In a multi-chunk setup with two outputs: ESM and CJS, where the chunk name patterns differ, Rollup may convert this:

import {Foo} from "./foo";
console.log(Foo);
export type Bar = typeof Foo;

To this for ESM

// index.esm.js
import {Foo} from "./chunk-0027dabcd....js";
console.log(Foo);

And this for CJS:

// index.cjs.js
const foo = require("./other-pattern-123456...");
console.log(foo.Foo);

Now, if we wanted to generate - and bundle - declarations for these two index files, it becomes clear that they are different.

The index.esm.d.ts file could look like this:

// index.esm.d.ts
import {Foo} from "./chunk-0027dabcd...";
export declare type Bar = typeof Foo;

And the index.cjs.d.ts file could look like this:

import {Foo} from "./other-pattern-123456...";
export declare type Bar = typeof Foo;

So under these circumstances, they would be different.

For by far the most library builds (though definitely not all of them), then yes, the declarations will be identical and only a single chunk will be emitted. I guess we could look into a heuristic saying something like "If all Rollup options like entryFileNames, chunkFileNames are identical across all output configs", then only generate declarations once".

[tunnckoCore]

It goes for both tsc and rollup-plugin-ts that you can provide a declarationDir option inside tsconfig.json to ensure that declarations are writing to a specific folder. However, there's no way of forcing a specific basename.

Yea yea, that's exactly what I did.

Would you be interested in this?

Don't it's needed at all for this case here - I'll try some things based on this PR. But might be a cool feature for some other cases.

But I don't see how we could avoid generating declarations for each Rollup output

Yea, I see it now more clearly.

It's possible, and the trick is quite easy. But TSDX does not support/emit/do anything chunks related anyway. So it's not the case here.

Basically the thing is that: for the one output format, use the plugin without providing tsconfig option, which means it will pick it up automatically, which must include (if you want declaration files at all) declaration: true and declarationDir typescript compiler options; and for the second output, pass a custom tsconfig path (which extends the base one) to the ts plugin, which specifically is adding declaration: false - that thing I called tsconfig.notypes.json.
And if we just put declarationDir: appDist/types or something like this, the tsc will error with that that this .d.ts file already exists, because the plugin is called twice. That's why we need to disable declaration generation for one output format.

[tunnckoCore]

So, basically I'll try to change this

https://github.com/medelman17/tsdx/blob/7f796bb66f906ad4182bef6f42b20656f52875d6/src/createRollupConfig.ts#L129-L138

to something like

      opts.format === 'esm'
      ? ts({
        tsconfig: tsconfig => ({
          ...tsconfig,
          target: ScriptTarget.ESNext,
          sourceMap: true,
          declaration: true,
          declarationDir: `${paths.appDist}/types`,
          jsx: JsxEmit.React,
        }),
        transpiler: 'babel',
      })
      : ts({
        tsconfig: tsconfig => ({
          ...tsconfig,
          target: ScriptTarget.ESNext,
          sourceMap: true,
          declaration: false,
          jsx: JsxEmit.React,
        }),
        transpiler: 'babel',
      }),

@@medelman17, got it? :)

And yet, it probably won't be index.d.ts, but still, at least it will be in predictable dir. Probably dist/types/foo.esm.prod.d.ts.... or.. Don't know it's still weird, haha.

Btw, @jaredpalmer, isn't it be better to always be index.<etc>.js (index.esm.prod.min.js) instead of "safe package name". Moving to that signature, will help here too. Also fan more to the directory separation (e.g. dist/cjs and dist/esm), instead of filename as currently and as react and others doing it.

[tunnckoCore]

So actually, @wessberg, this hook you are talking about will be in help here.

[tunnckoCore]

Anyway, forget about all the stuff I said. The whole thing boils down to that we use "safe package name" for output filename, instead of index which is definitely better. And second, the safePackageName function isn't that good. It completely strips the scope - in babel hypothetically case dist will be core.<etc>.js, in my scoped @hela/core package it will be the same, which isn't okay. At least we can include the scope and replace / with -. But overall, it makes things unpredictable and error/confusion-prone.

If user doesn't use the tsdx create he'll end up wondering what his typings file is. If we have the following output structure it will be a lot easier.

dist/index.js - conditional require, the `main` field points here
dist/cjs/index.js - normal
dist/cjs/index.min.js - production
dist/esm/index.js - normal, the `module` field points here
dist/esm/index.min.js - production

I don't understand why we need the package name, "production", "development" and the format in the filename. Always wondered that. It's hard on eyes. Not to mention that if it's "min" it's obviously for production (whatever that may mean in the different cases), right?

To be able to have single index.d.ts file we'll need to do the above-mentioned trick, checking what format is and if it's not "min". Otherwise, we'll end up having dist/cjs/index.d.ts and dist/esm/index.d.ts which will be the same file. Oh, and dist/cjs/index.min.d.ts and dist/esm/index.min.d.ts. And I'm assuming that we won't be able to have such .d.ts file (e.g. dist/index.d.ts which loads the cjs or esm d.ts file) as the dist/index.js, not to mention it does not make sense to have it, haha.

[tunnckoCore]

Seems good, right?

So, the users can for sure bet on this package.json fields

{
  "main": "dist/index.js",
  "module": "dist/esm/index.js",
  "typings": "dist/types/index.d.ts"
}

[wessberg]

So actually, @wessberg, this hook you are talking about will be in help here.

Alright, I've implemented such a hook and released it under v1.1.65.
I've documented it here

[tunnckoCore]

@wessberg cool! I'll try it.

---

offtopic: anytime I'm visiting this PR I'm wondering why it's called that way, @medelman17? 🤔 🤣
I don't get it. What it has to do with async/await? :D

[tunnckoCore]

@wessberg, Yup, it works great. But still, don't think it's for our case here :)
@jaredpalmer and @medelman17 call.

[tunnckoCore]

After above requested changes:

~/github/medelman17/tsdx/test/fixtures/build-default chore/swap-rollup-ts-plugins* 13s
❯ ls dist    
build-default.cjs.development.js         build-default.esm.js                  build-default.umd.production.min.js
build-default.cjs.development.js.map     build-default.esm.js.map              build-default.umd.production.min.js.map
build-default.cjs.production.min.js      build-default.umd.development.js      index.d.ts
build-default.cjs.production.min.js.map  build-default.umd.development.js.map  index.js

So, yea. We can go with this PR or #219 which suggest changing the dist structure.

[jaredpalmer]

Which PR are we going with?

[tunnckoCore]

Your call. @medelman17 needs to make changes here.

[designorant]

Which PR are we going with?

I'd go with this one purely for the benefit of having a full history here, even though @tunnckoCore has taken everything to a new level, really. Nevertheless, it looks like @medelman17 has been busy lately.

What it has to do with async/await?

@tunnckoCore It's explained in the first paragraph of #200 it addresses.

PS. Thank you all! <3

[medelman17]

I can make the needed changes to mine early tomorrow. That said, and while it would be great to have my contribution forever immortalized on my GH profile, I’m pretty sure I’m the only one who will notice. :-) So, let’s just go with whatever makes the most sense for the library and not make it emotional. If that’s the other PR, no problem. To the extent we can merge the two together and that would make sense, awesome. But ... whichever way, let’s get ‘er done. :-) Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Michał Ordon <notifications@github.com> Sent: Saturday, October 5, 2019 8:36:43 PM To: jaredpalmer/tsdx <tsdx@noreply.github.com> Cc: Michael Edelman <michael@fabulas.io>; Mention <mention@noreply.github.com> Subject: Re: [jaredpalmer/tsdx] Support Async/Await in Rollup Plugins (#208) Which PR are we going with? I'd go with this one purely for the benefit of having a full history here, even though @tunnckoCore<https://github.com/tunnckoCore> has taken everything to a new level, really. Nevertheless, it looks like @medelman17<https://github.com/medelman17> has been busy lately. What it has to do with async/await? @tunnckoCore<https://github.com/tunnckoCore> It's explained in the first paragraph of #200<#200> it addresses. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#208?email_source=notifications&email_token=ADQ3VLP6TRJEDGA45HHCZBLQNEXJXA5CNFSM4IWK7QZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAN6ZFI#issuecomment-538700949>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADQ3VLNX6OSUNQMMP3HRDXLQNEXJXANCNFSM4IWK7QZQ>.

[medelman17]

Alrighty. Feedback/changes from @tunnckoCore have been incorporated. :-)

[tunnckoCore]

Cool. But still, I think that we should somehow drop the dependency rollup-plugin-babel which mean we should move the tsdxPluginBabel (or how it was named) things (the babel config) as option passed to ts({ babelConfig }).

[jaredpalmer]

Going with this as it is not a breaking change. We can debate the output/safe-variable name elsewhere.

[jaredpalmer]

Thank you @medelman17 @tunnckoCore. You are both awesome.

[agilgur5]

@allcontributors please add @medelman17 for code, ideas

[allcontributors]

@agilgur5

I've put up a pull request to add @medelman17! 🎉

[agilgur5]

@allcontributors please add @tunnckoCore for reviews, code, ideas

[allcontributors]

@agilgur5

I've put up a pull request to add @tunnckoCore! 🎉

[agilgur5]

@allcontributors please add @wessberg for questions

[allcontributors]

@agilgur5

I've put up a pull request to add @wessberg! 🎉

[agilgur5]

Thanks everyone for helping with this!! (even if it eventually got rolled back)

To close this out for any readers that are unsure -- can see #294 (comment) and #506 for how I ended up fixing/getting this fixed upstream so that async Rollup plugins can be used.