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

Add type definitions for acorn.mjs #954

Merged
merged 4 commits into from May 29, 2020
Merged

Conversation

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented May 27, 2020

For TypeScript projects that use the acorn.mjs distribution bundle,
a separate .mjs.d.ts needs to be added to the folder. This was
suggested in microsoft/TypeScript#27957 (comment)

In DevTools, we use this in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2218015
to make it compile with TypeScript

For TypeScript projects that use the acorn.mjs distribution bundle,
a separate `.mjs.d.ts` needs to be added to the folder. This was
suggested in microsoft/TypeScript#27957 (comment)

In DevTools, we use this in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2218015
to make it compile with TypeScript
.gitignore Show resolved Hide resolved
@RReverser
Copy link
Member

@RReverser RReverser commented May 27, 2020

Is it possible to reuse these definitions in acorn.d.ts by re-exporting from one to another (in whatever direction)? My understanding is that they should work equally well for CommonJS?

@marijnh
Copy link
Member

@marijnh marijnh commented May 27, 2020

Agreed, I really don't want to have two copies of the same file in the repository.

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented May 28, 2020

Should be fixed now. Initially I tried to import the acorn.d.ts in acorn.mjs.d.ts, but realized that is not possible because ESM can't import commonjs. However, the reverse is possible. So if we define all types in acorn.mjs.d.ts, then acorn.d.ts can import them.

However, this would require users to add esModuleInterop to their tsconfig.json. I locally verified on a project that would use acorn with commonjs that making this change will still make TypeScript happy. So this technically is a breaking change after all 😢 Not sure how to work around that, let me know what you think.

acorn/dist/acorn.d.ts Outdated Show resolved Hide resolved
@RReverser
Copy link
Member

@RReverser RReverser commented May 28, 2020

Should be fixed now. Initially I tried to import the acorn.d.ts in acorn.mjs.d.ts, but realized that is not possible because ESM can't import commonjs.

From a quick attempt, putting this in acorn.mjs.d.ts seems to work:

import acorn from "./acorn.js";
export = acorn;

As far as I understand, it would also preserve backward compatibility - please correct me if I'm wrong and it's subject to the same issue.

@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented May 29, 2020

Yes it works, but I had to set allowSyntheticDefaultImports in our tsconfig.json to make it work. I think that is an acceptable workaround for us. Thanks @RReverser for the suggestion.

@RReverser
Copy link
Member

@RReverser RReverser commented May 29, 2020

Ah interesting. Not sure if there's a generic way to work around that...

@RReverser
Copy link
Member

@RReverser RReverser commented May 29, 2020

Actually... it seems that

import * as acorn from "./acorn";
export = acorn;

works just as well even without the flag?

This removes the need for an extra tsconfig flag
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented May 29, 2020

I don't know why this now works, as I tried before. But yes, this removes the need for any tsconfig change AND is not a breaking change. Awesome, thanks!

@RReverser
Copy link
Member

@RReverser RReverser commented May 29, 2020

Great, thanks for the PR!

@RReverser RReverser merged commit f66c4e7 into acornjs:master May 29, 2020
@TimvdLippe TimvdLippe deleted the TimvdLippe:add-types branch May 29, 2020
devtools-bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request May 29, 2020
The `.mjs.d.ts` file has been upstreamed in
acornjs/acorn#954 as per the instructions in
microsoft/TypeScript#27957 (comment)

R=jacktfranklin@chromium.org,bmeurer@chromium.org

Bug: 1011811
Change-Id: Ia2e0c3145318ac93b5e49d2b40780ffe7c469bab
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2218015
Commit-Queue: Tim van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Jack Franklin <jacktfranklin@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
babot pushed a commit to binaryage/dirac that referenced this pull request May 31, 2020
The `.mjs.d.ts` file has been upstreamed in
acornjs/acorn#954 as per the instructions in
microsoft/TypeScript#27957 (comment)

R=jacktfranklin@chromium.org,bmeurer@chromium.org

Bug: 1011811
Change-Id: Ia2e0c3145318ac93b5e49d2b40780ffe7c469bab
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2218015
Commit-Queue: Tim van der Lippe <tvanderlippe@chromium.org>
Reviewed-by: Jack Franklin <jacktfranklin@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.