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 typescript build #80

Merged
merged 15 commits into from
Dec 14, 2021
Merged

Add typescript build #80

merged 15 commits into from
Dec 14, 2021

Conversation

cmdcolin
Copy link
Contributor

Possible method to upgrade @gmod/vcf to use typescript

Adds a "variant class" that keeps a handle to the parser object. Adds a toJSON method that should be used instead of spreading the variant object directly (which is done in VcfFeature) because that spreads the parser contents also. Could try to workaround this if this is undesirable, but there is a small change to the jbrowse 2 codebase for the VcfFeature->toJSON method.

@garrettjstevens
Copy link
Contributor

I think that change is probably fine with a major version bump.

@cmdcolin cmdcolin force-pushed the add_tsc branch 4 times, most recently from 67a571e to c8c6c87 Compare December 3, 2021 15:47
@cmdcolin
Copy link
Contributor Author

cmdcolin commented Dec 3, 2021

This PR actually now has exact snapshot matches to the current main branch which is nice, uses private fields on the variant class to accomplish this

this has a minimum build ES2015 (unable to compile down to es5)

also the documentation module did not understand the private fields

produced error

yarn build
yarn run v1.22.15
$ npm run docs && npm run clean && npm run lint

> @gmod/vcf@5.0.2 docs
> documentation readme src/parse.ts --section API --parse-extension ts

SyntaxError: Unexpected token, expected ";" (13:9)
    at _class.raise (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:3939:15)
    at _class.unexpected (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:5248:16)
    at _class.semicolon (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:5232:40)
    at _class.parseClassPrivateProperty (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:8106:10)
    at _class.pushClassPrivateProperty (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:8083:30)
    at _class.parseClassMemberWithIsStatic (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:8009:14)
    at _class.parseClassMemberWithIsStatic (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:10001:58)
    at _class.parseClassMember (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:7948:10)
    at _class.parseClassMember (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:9958:46)
    at _class.parseClassBody (/home/cdiesh/src/gmod/vcf-js/node_modules/documentation/node_modules/@babel/parser/lib/index.js:7903:12) {
  pos: 266,
  loc: Position { line: 13, column: 9 }
}

The line
loc: Position { line: 13, column: 9 }

Is exactly the line in src/parse.ts that the private field is used

So, documentation module removed for now

Copy link
Contributor

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

Looks good! Glad to see this get TypeScript-ified.

I was able to get documentation-js working, see here: add_tsc...add_tsc_gs, along with just a couple minor things to help avoid ts-ignores.

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Dec 3, 2021

excellent :)

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Dec 3, 2021

merged this branch here

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Dec 4, 2021

I was trying to use this branch under node.js and it was sufficiently difficult (since neither es2015 or es2018 are commonjs formats) and using ESM under nodejs is somewhat difficult to use (because it REQUIRES you to have extensions on your imports e.g. import THING from './filenameMustHaveExtension.js' see nodejs/node#30927) so keeping it an es5 option may improve usability, and it is not possible to have that if using the private fields

Therefore I made this branch go back to the dynamically created class type that is on the current master branch for this repo 3e31a3b

@cmdcolin cmdcolin merged commit 7b59231 into master Dec 14, 2021
@cmdcolin cmdcolin deleted the add_tsc branch December 14, 2021 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants