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

feat: add deno entry #4

Merged
merged 5 commits into from Aug 11, 2020
Merged

feat: add deno entry #4

merged 5 commits into from Aug 11, 2020

Conversation

lukeed
Copy link
Owner

@lukeed lukeed commented Aug 10, 2020

First pass at a Deno entry.
All fs.* operations are available under Deno namespace.
Only had to import std:path module.

Questions:

  1. Both Deno.* operations require the allow-read permission. Is this something I have to flag on my side?
  2. Is there a better/built-in way to go from AyncIterator to Array?
  3. Do I have to add /deno to my package.json files? Assume not since publishing works thru git releases only

//cc @bcoe


Closes #3

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2020

Codecov Report

Merging #4 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #4   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           20        20           
=========================================
  Hits            20        20           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24efb4b...230627b. Read the comment docs.

@bcoe
Copy link

bcoe commented Aug 11, 2020

Both Deno.* operations require the allow-read permission. Is this something I have to flag on my side?

I've been capturing the error, and providing a useful note for users. It sounds like there is a way to prompt for permissions, might be a good option too:

https://dev.to/bmorearty/better-deno-security-ask-for-permission-at-runtime-1fnm

Is there a better/built-in way to go from AyncIterator to Array?

I'm not sure. Your approach seemed pretty reasonable to me 👍

Do I have to add /deno to my package.json files? Assume not since publishing works thru git releases only

No, you should be able to ignore /deno.

Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This is looking good 👍 for yargs, I will need the sync option

I think, I'd be happy to pitch in if you run out of cycles this week.

@lukeed
Copy link
Owner Author

lukeed commented Aug 11, 2020

Thanks! I'll add extra sync entry. I figured everything would be async in Deno because of top-level await but obviously that was a bad assumption.

I think permissions is all that's left to solve. Catching it makes sense for a yargs but I don't think it would for every module down the tree.

Happy to focus on it this week. Would hate to be the blocker for your conversion effort

@bcoe
Copy link

bcoe commented Aug 11, 2020

@lukeed yes, let me know where you land on permissions, this is something I'm trying to learn to navigate too.

@lukeed
Copy link
Owner Author

lukeed commented Aug 11, 2020

Hey, so I asked the Deno Discord and the recommendation is to allow the permission errors to happen. Any required permissions should be listed in the README (or in the functions' jsdoc blocks), and it's ultimately up to the consumer to add the appropriate flags. This is also how you can get method/usage-driven granular permissions.

I found feature proposals in the Deno repo about adding manifests/import-map-like files to define the permissions ahead of time, but there's not been any action taken on them yet. Design still WIP

@lukeed lukeed merged commit b47f528 into master Aug 11, 2020
@lukeed lukeed deleted the feat/deno branch August 11, 2020 05:01
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.

Feature request: Compatibility with Deno
3 participants