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

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

Merged
merged 2 commits into from
Oct 11, 2019
Merged

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

merged 2 commits into from
Oct 11, 2019

Conversation

medelman17
Copy link
Contributor

@medelman17 medelman17 commented Sep 13, 2019

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.

Screen Shot 2019-09-12 at 8 23 31 PM

closes #200

Copy link

@tunnckoCore tunnckoCore left a comment

Choose a reason for hiding this comment

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

That's it.

src/createRollupConfig.ts Show resolved Hide resolved
src/createRollupConfig.ts Outdated Show resolved Hide resolved
@medelman17
Copy link
Contributor Author

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.

@medelman17 medelman17 changed the title [WIP]: Support Async/Await in Rollup Plugins Support Async/Await in Rollup Plugins Sep 28, 2019
@wessberg
Copy link

wessberg commented Sep 28, 2019

@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
Copy link
Owner

Nice

@timReynolds
Copy link

timReynolds commented Sep 29, 2019

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

@tunnckoCore
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link
Contributor Author

medelman17 commented Sep 30, 2019 via email

@tunnckoCore
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link

wessberg commented Sep 30, 2019

@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
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link

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

@tunnckoCore
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link

Seems good, right?

2019-09-30-193623_1280x1024_scrot
2019-09-30-193720_1280x1024_scrot

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
Copy link

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
Copy link

tunnckoCore commented Sep 30, 2019

@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
Copy link

tunnckoCore commented Sep 30, 2019

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

src/index.ts Outdated Show resolved Hide resolved
test/tests/tsdx-build.test.js Outdated Show resolved Hide resolved
src/createRollupConfig.ts Outdated Show resolved Hide resolved
@tunnckoCore
Copy link

tunnckoCore commented Sep 30, 2019

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
Copy link
Owner

Which PR are we going with?

@tunnckoCore
Copy link

Your call. @medelman17 needs to make changes here.

@designorant
Copy link

designorant commented Oct 6, 2019

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
Copy link
Contributor Author

medelman17 commented Oct 6, 2019 via email

fix: tweak plugin and build config re types/declarations

fix: remove console.log; reimplement babel-plugin

chore: resolve PR comments
@medelman17
Copy link
Contributor Author

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

@tunnckoCore
Copy link

tunnckoCore commented Oct 6, 2019

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
Copy link
Owner

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

@jaredpalmer jaredpalmer merged commit bfc0590 into jaredpalmer:master Oct 11, 2019
@jaredpalmer
Copy link
Owner

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

bitmoji

@medelman17 medelman17 deleted the chore/swap-rollup-ts-plugins branch October 11, 2019 21:15
@agilgur5 agilgur5 changed the title Support Async/Await in Rollup Plugins Support Async/Await in Rollup Plugins by migrating to @wessberg/rollup-plugin-ts Mar 16, 2020
@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
Copy link
Collaborator

@allcontributors please add @medelman17 for code, ideas

@allcontributors
Copy link
Contributor

@agilgur5

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

@agilgur5
Copy link
Collaborator

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

@allcontributors
Copy link
Contributor

@agilgur5

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

@agilgur5
Copy link
Collaborator

@allcontributors please add @wessberg for questions

@allcontributors
Copy link
Contributor

@agilgur5

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

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 16, 2020

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.

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.

Migrate Rollup TS Plugin to '@wessberg/rollup-plugin-ts' to support async rollup plugins
7 participants