NODE-2716: Convert to Typescript #388

nbbeeken · 2020-08-19T16:55:42Z

Converted the codebase to typescript. In order to get us up and running with testing I also did the leg work on bringing our tooling in sync with the driver, (for example we have to update prettier to get import type syntax). I brought in ts-node and got that working with mocha so tests are still being run and this works for both Node ~~and the in browser karma tests~~ (if the bundle is broken there's no karma tests! spoke too soon). This initial run is minimal changes needed to get us a compiling code base, unlike when we did this for the driver I opted to initially disable typescript's strict rule which allows me to omit type annotations on functions mainly, which in a subsequent fix up PR would all be changed from any to something more meaningful.

I also brought in our gulp file from the driver, along with the api-extractor config so we are ready to go on distributing typings.
Another pass will be to add the ts-doc information about access level and correctly formatting the @param tags etc.

Currently this PR will break our rollup config, I'm working on getting that working with the new TS code but figured I get the ball rolling on this initial step.

nbbeeken · 2020-08-19T19:10:11Z

Ah so something I realized after the fact, currently when you npm install bson you get a library that has a lib folder and a dist folder. The dist folder contains the bundles intended for the web and the lib folder was just our source, I assume we want to keep that form of distribution? I got mixed up I guess because we're now generating the lib folder. And I can generate the bundles from the generated lib folder instead of directly from the ts which works without any changes to the current rollup setup. (At least I'm pretty sure, I can revert to make sure, but that's why karma originally passed my tests without complaint, cus it was using the compiled lib folder!)
OR we can make sure our bundles are generated directly from typescript. This is fine and I'm expecting it to be preferred.

In either case:
There's the issue that was found with the es module formatted bundles, this issue is unfixable with the current method that the 'long' dependency uses to export the class module.exports = Long is incompatible with import Long from 'long'. And if you attempt to use import * as Long from 'long' the result is:

import * as Long from 'long';
import { prototype, fromString as fromString$1, fromNumber, ZERO, isLong, fromInt, fromBits } from 'long';

Which the first line give you hope for a solution but alas the second line shows you that pulling apart the class into its individual components like prototype is not what we desire.

As an aside, rollup (at least the rollup of today), is designed to take in esm style code and translate that to the other formats (commonjs, umd, iife) however we have a codebase written in commonjs and rely on rollup to translate that to esm. That's not in itself an issue its just the inverse of what the bundler tool is designed for today.

mbroadst

Lookin great so far! I have a few questions below. I'm also wondering if we shouldn't try to make jsdoc => typedoc changes here, even if the types aren't in place everywhere, just to reduce the amount of future churn

Gulpfile.ts

src/db_ref.ts

src/document.ts

mbroadst · 2020-08-20T13:51:38Z

src/map.ts

+if (typeof global.Map !== 'undefined') {
+  exportedMap = global.Map;
+} else {
+  exportedMap = require('core-js/es/map');


hm shouldn't this otherwise be polyfilled by TypeScript for us?

Typescript doesn't do polyfills, that's a babel thing which is strange since we use babel to shim Buffer in browsers, we could have used babel to polyfill Map, that might still be a possible solution here instead of manually importing it (I loathe the directly added dependency). Although on the other hand perhaps we shouldn't be doing this logic of if Map exists on global and instead export the polyfill regardless since the point here is to use it on platforms where Map is not supported.

Not sure if this is covered in the bundling but browser would need window.Map.

src/symbol.ts

src/timestamp.ts

nbbeeken · 2020-08-20T17:52:11Z

I have not found a solution to using Long without vendoring it and modifying it to use esm syntax, (I think I could also just modify it to do module.exports.Long = Long, key change being the Long key on the exports object, but I figured if I'm vendoring anyway, take it the full mile).

I still need to fix up the gulp file to use plugins correctly and the rollup config still has issues with correctly shimming Buffer.

src/validate_utf8.ts

reggi · 2020-08-24T20:32:02Z

Just noticed some places for code / logic improvement we can hold off on.

I also saw some untyped arguments, are we leaving these untyped and doing more incremental progress?

nbbeeken · 2020-08-26T17:40:49Z

There's an issue with testing, I can confirm locally that tests pass on node 6.8 but tsc supports node 10+ so I can work on updating the EVG shell files to get both versions of node, and compile with one and test with the other but that might be best to leave to another pr, let me know what's best

package.json

src/objectid.ts

src/db_ref.ts

src/timestamp.ts

rollup.config.js

.evergreen/install-dependencies.sh

tools/scenarios-plugin.js

test/register-bson.js

DBRefShape -> DBRefLike stop using `export *` to avoid tslib dep approximate _bsontype on timestamp

remove unneeded plugins

…ript (#388)" This reverts commit bdc046f.

…ript (#388)" (#390) This reverts commit bdc046f.

…o Typescript (#388)" (#390)" This reverts commit 314c05d.

chore: convert to typescript

5c07f28

nbbeeken requested review from reggi, emadum and mbroadst August 19, 2020 16:55

fix: lint script and wip rollup fixes

9092f6b

mbroadst reviewed Aug 20, 2020

View reviewed changes

progress update on bundling and compiling

fb3c5c4

reggi reviewed Aug 24, 2020

View reviewed changes

src/validate_utf8.ts Show resolved Hide resolved

reggi approved these changes Aug 24, 2020

View reviewed changes

nbbeeken added 11 commits August 25, 2020 10:42

fix: bundling works, karma does not

344626a

wip: test fixes

93170b5

Take public out of symbol constructor

efc75fd

fix: npm scripts

daa971e

fix: add helpers dep fix engine version

04bfef0

remove extra document file, remove terser

6b7eddd

it works

faa6291

fix missing dependency

db4cec2

fix node 10 issue

cc0d560

fix: node 6 issues

ac0e53f

flatten bson.d.ts emit to root

b0f5e18

nbbeeken requested review from mbroadst and reggi August 26, 2020 17:30

nbbeeken added 3 commits August 27, 2020 10:52

fix: windows tests issue

7f71c70

fix: older versions of node test issues

06acd43

fix: use explicit lts version number

00987ec

mbroadst suggested changes Aug 28, 2020

View reviewed changes

nbbeeken added 3 commits August 28, 2020 11:49

bring back old Map polyfill

872ccd4

DBRefShape -> DBRefLike stop using `export *` to avoid tslib dep approximate _bsontype on timestamp

change long back to using defineProperty for _bsontype

9d23c07

remove unneeded plugins

Windows nvm doesn't support lts alias

cee4a1f

nbbeeken requested a review from mbroadst August 31, 2020 14:32

reggi approved these changes Sep 1, 2020

View reviewed changes

mbroadst approved these changes Sep 1, 2020

View reviewed changes

fix: prettier-ignore WASM

330b0ae

nbbeeken merged commit bdc046f into master Sep 1, 2020

nbbeeken deleted the NODE-2716/ts-bson branch September 1, 2020 19:20

nbbeeken added a commit that referenced this pull request Sep 11, 2020

Revert "NODE-2716, NODE-2718, NODE-2719, NODE-2721: Convert to Typesc…

f7d4be7

…ript (#388)" This reverts commit bdc046f.

mbroadst pushed a commit that referenced this pull request Sep 14, 2020

Revert "NODE-2716, NODE-2718, NODE-2719, NODE-2721: Convert to Typesc…

314c05d

…ript (#388)" (#390) This reverts commit bdc046f.

nbbeeken added a commit that referenced this pull request Sep 14, 2020

Revert "Revert "NODE-2716, NODE-2718, NODE-2719, NODE-2721: Convert t…

0c469fd

…o Typescript (#388)" (#390)" This reverts commit 314c05d.

NODE-2716: Convert to Typescript #388

NODE-2716: Convert to Typescript #388

Uh oh!

Conversation

nbbeeken commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbbeeken commented Aug 19, 2020

Uh oh!

mbroadst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbroadst Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

nbbeeken Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

reggi Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nbbeeken commented Aug 20, 2020

Uh oh!

Uh oh!

reggi commented Aug 24, 2020

Uh oh!

nbbeeken commented Aug 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nbbeeken commented Aug 19, 2020 •

edited

Loading