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

Map-like interface Deno.env #4942

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Map-like interface Deno.env #4942

merged 1 commit into from
Apr 29, 2020

Conversation

SyrupThinker
Copy link
Contributor

As noted in #4933

cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/ops/os.ts Outdated Show resolved Hide resolved
@SyrupThinker SyrupThinker marked this pull request as ready for review April 27, 2020 22:34
@SyrupThinker SyrupThinker changed the title WIP More Map-like interface Deno.env Map-like interface Deno.env Apr 27, 2020
cli/js/ops/os.ts Outdated Show resolved Hide resolved
cli/js/ops/os.ts Outdated Show resolved Hide resolved
@ry ry mentioned this pull request Apr 27, 2020
43 tasks
@ry ry requested a review from piscisaureus April 28, 2020 13:07
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

I'm happy with this API. Leaving it to @piscisaureus for final review

@ry
Copy link
Member

ry commented Apr 28, 2020

I was wondering if we could call it entries instead of getAll while still returning an object. Isn’t that an iterator still?

@bartlomieju
Copy link
Member

I was wondering if we could call it entries instead of getAll while still returning an object. Isn’t that an iterator still?

entries() return iterator object (the one that has next() method); plain objects don't have iterator implementation

EDIT: Oooops I somehow edited @ry's comment by mistake

@ry
Copy link
Member

ry commented Apr 29, 2020

getAll() sounds strange to me.

A couple of other possibilities:

  • forEach method to bring it inline with Map.
  • entries method which returned a Array<[string, string]> - this is an iterable object
  • toObject() method which does what getAll is doing now.

none of these are particularly great...

@bartlomieju
Copy link
Member

getAll() sounds strange to me.

A couple of other possibilities:

  • forEach method to bring it inline with Map.
  • entries method which returned a Array<[string, string]> - this is an iterable object
  • toObject() method which does what getAll is doing now.

none of these are particularly great...

The best one would be entries() - but to turn it into object, one would need to wrap it additionally in Object.fromEntries()

const env = Object.fromEntries(Deno.env.entries());

vs 

const env = Deno.env.getAll();

Just for reference; in Rust:

let env = env::vars(); (returns interator)

let envVar = env::var("my_var");

env::set_var("my_var", "my_value");

env::remove_var("my_car");

@ry
Copy link
Member

ry commented Apr 29, 2020

I agree that entries is ideal too. What if we just did toObject() for now, and add a TODO for entries()?

@SyrupThinker
Copy link
Contributor Author

SyrupThinker commented Apr 29, 2020

Wouldn't the entries() implementation basically be Object.entries(Deno.env.toObject())?

@bartlomieju
Copy link
Member

After further discussion we've settled on get()/set()/toObject(). @SyrupThinker can you update the PR?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @SyrupThinker !

@ry ry merged commit 721a4ad into denoland:master Apr 29, 2020
@SyrupThinker SyrupThinker deleted the deno_env branch May 1, 2020 16:27
@jakajancar
Copy link

What's the benefit of the Map-like interface over Deno.env simply being an object already?

console.log(Deno.env.SOMETHING);
Deno.env.SOMETHING_ELSE = "BLAH";

@ry
Copy link
Member

ry commented May 3, 2020

@jakajancar We want to make it look less like you're updating some object - but rather calling functions which have side-effects. In particular, in Windows the env vars are not case sensitive.

@jakajancar
Copy link

Ah, the Windows situation is reason enough. That would indeed look odd if properties changed in unexpected ways.

Thanks for the explanation!

SASUKE40 pushed a commit to SASUKE40/deno that referenced this pull request May 7, 2020
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
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.

4 participants