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: deno_std node addition #3319

Merged
merged 12 commits into from
Nov 12, 2019
Merged

feat: deno_std node addition #3319

merged 12 commits into from
Nov 12, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth commented Nov 11, 2019

# Deno Node compatibility

This module is meant to have a compatibility layer for the
[nodeJS standard library](https://nodejs.org/docs/latest-v11.x/api/).
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be TLS version

Copy link
Contributor

Choose a reason for hiding this comment

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

*LTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no link directly pointing to LTS version, updated it to aim v12 as it's LTS: https://nodejs.org/en/

Copy link
Contributor

@axetroy axetroy left a comment

Choose a reason for hiding this comment

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

I don't think it's a good time to merge it into deno.

At least, there is no relative discussion.

@kevinkassimo
Copy link
Contributor

@axetroy Actually there was some basic discussion in Gitter chatroom.
Moving to std serves to expose this better for contributions and gaining more traction.

std/node/util.ts Outdated
isObject,
isError,
isFunction
} from "./util/main.ts";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this from main.ts to mod.ts.

I am not quite sure if we should reexpose mod as std/node/util.ts or just leave as std/node/util/mod.ts. The latter follows our recommendation better, while the former might seem more familiar to node users...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mod.ts seems better yes.
Regarding the exposure it's more like a taste, you have 2 solutions
import { fs } from 'std/node' or import { readfile } from 'std/node/fs' i'm ok with both

This module is meant to have a compatibility layer for the
[nodeJS standard library](https://nodejs.org/docs/latest-v11.x/api/).

**Warning** : Any function of this module do not have to be refered anywhere in
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is "should not" instead of "do not have to"?
std/node should be able to import utilities from other std modules, but not the other way around IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: referred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this is "should not" instead of "do not have to"?
std/node should be able to import utilities from other std modules, but not the other way around IMO.

that's what i was meaning. Sorry for my engrish

@zekth
Copy link
Contributor Author

zekth commented Nov 12, 2019

I don't think it's a good time to merge it into deno.

At least, there is no relative discussion.

Ref: https://gitter.im/denolife/Lobby?at=5dbda5dbe1c5e915081e2729

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.

Looking good ...

std/node/fs.ts Outdated
@@ -0,0 +1 @@
export { readFile, readFileSync } from "./fs/read_file.ts";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just merge read_file.ts into this one - it will make for better docs on Deno.land and also I prefer to keep the number of files small until it becomes unwieldy to have so many functions in a single file - at that point we can break it out. So just fs.ts and fs_test.ts please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was just a direct port but i get your point.

@@ -0,0 +1,122 @@
import { test } from "../../testing/mod.ts";
import { assert } from "../../testing/asserts.ts";
import * as util from "./main.ts";
Copy link
Member

Choose a reason for hiding this comment

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

main.ts ? I don’t think we should use the file name main.ts in std/node ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, direct port, i'll change this as mentionned in the other comment

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 - hopefully this gets more attention in std. I kinda want to add std/node/require.ts now.

@ry ry merged commit ee1b8dc into denoland:master Nov 12, 2019
@zekth zekth deleted the add_denolib_node branch November 12, 2019 22:26
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Dec 28, 2019
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.

5 participants