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

Migrate JSBI source to TypeScript. #67

Merged
merged 3 commits into from
Aug 23, 2021

Conversation

12wrigja
Copy link
Collaborator

This is based off similar work I did for js-temporal/temporal-polyfill#21.

Open questions:

  • how does changing "type" in package.json potentially affect JSBI users?
  • does the JSBI source need to be moved into a folder in order for the sourcemaps to end up correct in the npm package?
  • does the TS source need to be included in the package as well (via the package.json files list?)
  • is es2015 "low-enough" of a tsc emit before passing to babel? Or should the tsc emit be a much higher level emit and let babel do the bulk of the transpilation?

@google-cla google-cla bot added the cla: yes label Aug 13, 2021
Copy link
Collaborator

@jakobkummerow jakobkummerow left a comment

Choose a reason for hiding this comment

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

Source changes look good, with a few minor comments.

What's with the (internal) functions that haven't received type annotations?

Hopefully Mathias can say more about the open questions.

Regarding target ES level: keep in mind that JSBI is squarely targeted at old browsers. We've had people specifically request IE11 compatibility fixes before. I'm not sure how they dealt with the fact that class JSBI is an ES2015 feature. So it might even make sense to target ES5. (For the majority of the code, it shouldn't make a difference anyway.)

lib/jsbi.ts Outdated Show resolved Hide resolved
lib/jsbi.ts Outdated Show resolved Hide resolved
lib/jsbi.ts Outdated Show resolved Hide resolved
lib/jsbi.ts Outdated Show resolved Hide resolved
lib/jsbi.ts Outdated Show resolved Hide resolved
@12wrigja
Copy link
Collaborator Author

The problem with es5 transpilation is that the tsc output for class extends Array is broken when transpiling to es5. See this TypeScript playground example as a minimal repro.

@12wrigja
Copy link
Collaborator Author

"What's with the (internal) functions that haven't received type annotations?" I had another branch with these and forgot to merge it in before pushing/reviewing. PTAL. I tried to infer the right types for things based on how they were used, but there's at least a couple places where the tsc control flow can't infer the right things, so some casts were necessary to retain the existing code.

@12wrigja
Copy link
Collaborator Author

@justingrant You did a great job finding and helping me figure out the source-mapping issues with the Temporal TS migration - would you mind taking a look at JSBI as well?

Copy link
Collaborator

@jakobkummerow jakobkummerow left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. I'll leave the packaging questions to other reviewers.

@mathiasbynens
Copy link
Member

how does changing "type" in package.json potentially affect JSBI users?

See inline comment. Can we avoid making this change?

does the JSBI source need to be moved into a folder in order for the sourcemaps to end up correct in the npm package?

Hmm, I’m not sure… Could you test with DevTools?

does the TS source need to be included in the package as well (via the package.json files list?)

No. The way I think about this is: the fact we’re using TS is an implementation detail, and does not influence the library that we end up publishing. We could include .d.ts typings in the package, however, since this is useful for developers.

is es2015 "low-enough" of a tsc emit before passing to babel? Or should the tsc emit be a much higher level emit and let babel do the bulk of the transpilation?

The current approach seems fine to me.

.eslintrc.cjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@12wrigja 12wrigja requested a review from mathiasbynens August 20, 2021 23:51
@@ -817,7 +817,7 @@ const dataSmall = [{
r: '0x7bffcbe01'
}];

import JSBI from '../jsbi.mjs';
Copy link
Member

Choose a reason for hiding this comment

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

It's annoying that TypeScript doesn't support this (I guess?), because it breaks d8 benchmarks/sub.mjs — we now have to always test the compiled output instead.

@mathiasbynens mathiasbynens merged commit 877f076 into GoogleChromeLabs:main Aug 23, 2021
@mathiasbynens
Copy link
Member

Released as v3.2.0.

mathiasbynens added a commit that referenced this pull request Aug 23, 2021
As part of the TypeScript migration in #67 we accidentally made the CommonJS bundle have a `default` export, which wasn’t the case before.

This patch fixes the Rollup configuration to restore the old behavior of `module.export = JSBI`.

Issue: #68, #67
mathiasbynens added a commit that referenced this pull request Aug 23, 2021
As part of the TypeScript migration in #67 we accidentally made the CommonJS bundle have a `default` export, which wasn’t the case before.

This patch fixes the Rollup configuration to restore the old behavior of `module.export = JSBI`.

Issue: #68, #67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants