fix(typescript): aggregator-error imported properly

[oscard0m]

📝 Summary

To apply proper import when using aggregate-error type

⛱ Motivation and Context

Fix #245

📊 How Has This Been Tested?

Running tsc works

Before

After

But when running npm run build(pika) does not:

📚 References:

https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require

[oscard0m]

@wolfy1339 @gr2m I will need some help here, seems like we have an incompatibility issue when importing aggregate-error type on types.ts

Any idea on how can we bypass this?

[wolfy1339]

Why not use the syntax recommended by the test action?
import * as ns from "mod"

[oscard0m]

Trying this approach I get the following error:

Also, trying import AggregateError from ... seems to be an issue:

---

The reason I was using import AggregateError = require() is because what I found in Typescript docs: https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require but not sure how to handle this.

• https://github.com/sindresorhus/aggregate-error/blob/d5bb4ac02a43f005ec7ad45f6e62919d7ebed0e5/index.d.ts#L51

---

Any ideas @wolfy1339

[wolfy1339]

We shouldn't be targetting commonjs, Pika does that for us.

If you use the syntax i commented above, then you don't need to use the TS import = require()

[gr2m]

The npm run validate:ts script is failing: https://github.com/octokit/webhooks.js/pull/248/checks?check_run_id=1072429167#step:5:1

This is so odd. Maybe we could research how other TypeScript projects are using aggregator-error? Or create a minimal test case and file an issue? I trust Sindre knows what he is doing, he has a bazillion repositories and added TypeScript definitions to most of them

[wolfy1339]

The problem here is that we are using ES Modules, and AggregateError is using CommonJS.

Normally, under regular plain JS using ES Modules, using the following syntax works fine

import * as AggregateError from 'aggregate-error'

However, in TypeScript, one needs to enable the esModuleInterop flag in order to do so.

There's also the "Synthetic default import" mode in TypeScript (allowSyntheticDefaultImports) which was the syntax that was used before.

import AggregateError from 'aggreagate-error'

This unfortunately, has nothing to do with upstream. It's a problem/feature of TS

[gr2m]

If this particular library is such a trouble, we could just copy/paste the source code, it's ~40LOC: https://github.com/sindresorhus/aggregate-error/blob/master/index.js and two small dependencies that we could replace or remove if necessary

[oscard0m]

If this particular library is such a trouble, we could just copy/paste the source code, it's ~40LOC: https://github.com/sindresorhus/aggregate-error/blob/master/index.js and two small dependencies that we could replace or remove if necessary

Before opting for this I would like to check how other dependents of this package are importing it and which tsconfig options are they using.

[oscard0m]

If this particular library is such a trouble, we could just copy/paste the source code, it's ~40LOC: https://github.com/sindresorhus/aggregate-error/blob/master/index.js and two small dependencies that we could replace or remove if necessary

Before opting for this I would like to check how other dependents of this package are importing it and which tsconfig options are they using.

Hey, just a followup on this topic. I just researched on several dependents from aggregate-error package and I did not find a case which fits our use case of trying to import as type and with ESModules considering the way this package is exporting it.

I just posted a question to aggregate-error, I would say to move forward in 24h with @gr2m suggestion of copying and adapting the export of aggregate-error to our needs until we get an answer.

I did a quick research on StackOverflow on this but I did not find a specific topic on this. Do you think it would be interesting to post the question there?

@wolfy1339 @gr2m

[gr2m]

Thank you for the research @dominguezcelada.

I'd suggest we do a local override of the types, as @smockle suggests here:
https://github.com/smockle/aggregate-error-test

Maybe we can make the override more explicit, by calling the folder src/@types-overrides/aggregate-error?

[gr2m]

Actually nevermind, let's just go ahead and remove the dependency altogether, and instead move the whole aggregate-error source code into @octokit/webhooks. We have to make sure to give credit and copy the license text to the top of the file: https://raw.githubusercontent.com/sindresorhus/aggregate-error/master/license. Also add an explanation of what we did.

I hope that TypeScript will fix the problem on the tooling side, at which point we will be able to remove the workaround

[oscard0m]

Actually nevermind, let's just go ahead and remove the dependency altogether, and instead move the whole aggregate-error source code into @octokit/webhooks. We have to make sure to give credit and copy the license text to the top of the file: sindresorhus/aggregate-error@master/license (raw). Also add an explanation of what we did.

I hope that TypeScript will fix the problem on the tooling side, at which point we will be able to remove the workaround

Sounds good to me. I will take care on getting some assistance with TypeScript and followup on this.

[oscard0m]

For what I've read on the thread discussion @gr2m shared and as DefinetelyTyped suggests, we are tied to how aggregate-error decides to expose their types (export = in this case).

If we want to use import AggregateError from 'aggregate-error' or import * as AggregateError from 'aggregate-error' it is necessary to use esModuleInterop or allowSyntheticDefaultImports, and doing so, would imply everybody using @octokit/webhooks.js to use this flag on their project too (as @wolfy1339 pointed).

So... here the solutions would be (or at least what I found):

• enable esInteropsModule flag as a breaking change (so a major release)
• to keep track on TypeScript releases + aggregate-error releases until this can be sort out without this flag (and stying with these changes (fix(typescript): workaround for trouble with aggregate-error type exports (#245) #270) in the meantime).

[gr2m]

It's also possible that the problem gets resolved within TypeScript 🤞🏼 Lets move forward with #270 and hope for the best

[gr2m]

resolved by #270