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

refactor!: typescript/ESM conversion #95

Merged
merged 9 commits into from
Sep 5, 2020
Merged

refactor!: typescript/ESM conversion #95

merged 9 commits into from
Sep 5, 2020

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Aug 5, 2020

convert module to using TypeScript.

@QmarkC I'm using the pattern that I developed in yargs-parser, and attempting to port this module to TypeScript in such a way that we can target both ESM and Deno.

My end goal is to do this with the dependency graph of yargs, such that this module would be fully compatible with ESM, Deno, and CJS.

TODO:

  • get all unit tests passing.
  • move fs/util/path modules to mixin, such that they work in all environments.

@bcoe bcoe requested a review from QmarkC August 5, 2020 16:33
@QmarkC
Copy link
Contributor

QmarkC commented Aug 6, 2020

@bcoe I pulled the typescript branch and I'm working on some changes/feedback for you.

@bcoe
Copy link
Member Author

bcoe commented Aug 8, 2020

@QmarkC awesome, want to take this PR over and I'll concentrate on yargs itself this weekend?

@QmarkC
Copy link
Contributor

QmarkC commented Aug 11, 2020

@QmarkC awesome, want to take this PR over and I'll concentrate on yargs itself this weekend?

Sure thing. I've got most of it done but still need to fix a couple items. I'll try to wrap it up in the next few days but might not have much time till this weekend.

@QmarkC
Copy link
Contributor

QmarkC commented Aug 12, 2020

@bcoe here's my WIP, master...QmarkC:85-typescript.
tests are passing but haven't gotten to move fs/util/path modules to mixin yet.

@bcoe
Copy link
Member Author

bcoe commented Aug 12, 2020

Sure thing. I've got most of it done but still need to fix a couple items. I'll try to wrap it up in the next few days but might not have much time till this weekend.

@QmarkC awesome, I don't expect I'll get yargs too much farther along until the weekend either.

tests are passing but haven't gotten to move fs/util/path modules to mixin yet.

Awesome 😄

In yargs' I ended up landing on the term "platform shims" rather than "mixin", which seemed perhaps a better description of what was happening (see: here).

I was thinking I'd then make an effort to write unit tests for these shims... what I care about most is that yargs itself has support for a variety of platforms, and is well tested.

@QmarkC
Copy link
Contributor

QmarkC commented Aug 12, 2020

In yargs' I ended up landing on the term "platform shims" rather than "mixin", which seemed perhaps a better description of what was happening (see: here).

I was thinking I'd then make an effort to write unit tests for these shims... what I care about most is that yargs itself has support for a variety of platforms, and is well tested.

For "mixin", I was just using your terminology. We could polyfill/shim with rollup plugins such as rollup-plugin-node-builtins or rollup-plugin-node-polyfills. The local files methods could be moved to another module with conditional exports based on environment or using different entry points per environment. For y18n I was thinking we could add support for loading a locale as an object via an optional config prop and/or adding an isomorphic fetch like cross-fetch, which might be better for browser support to allow loading remote locale files.

@bcoe
Copy link
Member Author

bcoe commented Aug 13, 2020

I was thinking we could add support for loading a locale as an object via an optional config prop and/or adding an isomorphic fetch like cross-fetch, which might be better for browser support to allow loading remote locale files.

@QmarkC the challenge here, is that yargs is poorly designed for an asynchronous model ... as soon as we require something to be async like the fetch operation, a bunch of other assumptions fall apart.

I've definitely of a mind that I'd like to get to a point where yargs expects you're using async/await to invoke it, at which point it opens up a variety of options -- this makes command handling way better too.

For "mixin", I was just using your terminology. We could polyfill/shim with rollup plugins such as rollup-plugin-node-builtins or rollup-plugin-node-polyfills.

I think there's value in us isolating the platform specific functionality ourselves, at the end of the day there's not a huge amount of it, and it suddenly puts us in great position to target a bunch of runtimes ... I was just making the point that maybe I could have chosen a better variable name, and that my code was getting slightly sloppy, so refactoring into a nicer abstraction might be a good idea (especially in yargs itself).

@bcoe bcoe merged commit 4d7ae94 into master Sep 5, 2020
@bcoe bcoe deleted the typescript branch September 5, 2020 00:20
@bcoe
Copy link
Member Author

bcoe commented Sep 5, 2020

@QmarkC landed a first pass and integrated it:

yargs/yargs#1735

I didn't ship the types though, in case you would like to improve them 👍 (figured this way we could add the types as feat: and perhaps not a breaking change).

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