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

Feature request: Compatibility with Deno #3

Closed
bcoe opened this issue Aug 8, 2020 · 9 comments · Fixed by #4
Closed

Feature request: Compatibility with Deno #3

bcoe opened this issue Aug 8, 2020 · 9 comments · Fixed by #4

Comments

@bcoe
Copy link

bcoe commented Aug 8, 2020

Hey @lukeed, I find myself revisiting this module, because I'm part way towards adding ESM support to yargs.

I have a feature request ...

Would you be open to using dependency injection in this library, similar to what I've done in yargs-parser, to target Deno as well?

The main advantage I see to this is that it helps isolate the Node-specific APIs you're using, making it easier to target a variety of other platforms.

@lukeed
Copy link
Owner

lukeed commented Aug 8, 2020

Hey :) welcome back, haha

Would a separate entry work? These are bad names, but something like escalade/deno or escalade/shim. I can add this without being breaking, since it's opt in.

Personally I've been waiting on Deno's import maps, or a "deno" package key to land, or for Deno itself to find another way to get Node natives to resolve without needing to import all of std/module into scope.

How is Deno able to resolve that entry file? Is it purely just through URL access?

@bcoe
Copy link
Author

bcoe commented Aug 8, 2020

Would a separate entry work? These are bad names, but something like escalade/deno or escalade/shim. I can add this without being breaking, since it's opt in.

How is Deno able to resolve that entry file? Is it purely just through URL access?

Exposing a separate entry point like ./deno.js or ./deno.ts (if you're so inclined) seems to work quite well. Then you can toss the library up here which requires:

  1. an initial OAuth authentication step/creation of WebHook.
  2. the tagging of releases (which you already are).

A consuming library can then do this

// the Deno.land registry URL could be used below alternatively:
import parser from "https://github.com/yargs/yargs-parser/raw/deno/deno.ts";

const argv = parser('--foo=99 --bar=9987930', {
  string: ['bar']
})
console.log(argv)

In the case of a library as simple as this, the main work would be isolating the fs operations:

fs.readdir -> Deno.readDir.
fs.stat -> Deno.stat.

etc.

@lukeed
Copy link
Owner

lukeed commented Aug 9, 2020

Cool! I'm glad the separate-file route is an option. My weekends are away from the computer lately, so I'll have this done by Monday at the latest. Does Deno support @next tags or will I have to risk extra versions if I get the upload wrong, haha

@bcoe
Copy link
Author

bcoe commented Aug 9, 2020

Does Deno support @next tags or will I have to risk extra versions if I get the upload wrong, haha

I believe right now it just makes whatever tag was most recently published the most recent version.

This isn't great if there's a build step ... for yargs-parser, I have a GitHub action that creates a special deno tag whenever a release happens:

https://github.com/yargs/yargs-parser/blob/master/.github/workflows/release-please.yml#L24

@lukeed
Copy link
Owner

lukeed commented Aug 11, 2020

Ok, everything seems to work locally @bcoe
When you get a chance, can you verify that things work as expected on your end too? If they do, I'll publish 3.1.0 and consider this done :)

This is the test script I used as a sanity check:

import escalade from 'https://deno.land/x/escalade/sync.ts';

let output = escalade('.', (dir, list) => {
	console.log('~> dir', dir);
	console.log('~> list', list);
	if (list.includes('package.json')) return 'package.json';
});

console.log('OUTPUT', output);
$ deno run --allow-read deno/test.ts

@lukeed
Copy link
Owner

lukeed commented Aug 12, 2020

I can visit find-up usage from your checklist if you're still looking for help on that item

@bcoe
Copy link
Author

bcoe commented Aug 13, 2020

@lukeed awesome work 👍 I have been a little slammed this week, but intend to integrate this weekend 👏

@lukeed
Copy link
Owner

lukeed commented Aug 16, 2020

Thank you :) lemme know if you need anything. Happy to chip in if needed

@bcoe
Copy link
Author

bcoe commented Aug 16, 2020

@lukeed things seem to be working great:

// TODO: we should think of a way to support this functionality
Deno.test('guesses version # based on package.json', () => {
  let output: string|null = null
  Yargs()
    .parse('--version', (_err: Error, argv: Arguments, _output: string) => {
      output = _output
    })
  assertMatch('' + output, /[0-9]+\.[0-9]+\.[0-9]+/)
})

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 a pull request may close this issue.

2 participants