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

Code samples #96

Merged
merged 14 commits into from
Feb 28, 2019
Merged

Code samples #96

merged 14 commits into from
Feb 28, 2019

Conversation

hemanth
Copy link
Member

@hemanth hemanth commented Jan 17, 2019

Code samples for few of the proposals.

P.S: I am having issues running the docker, pinged on IRC.

_data/stage3.yml Outdated
@@ -225,6 +254,15 @@
champions:
- Jordan Harband
- Kevin Gibbons
example: >
const entries = new Map([
Copy link
Member

@ljharb ljharb Jan 17, 2019

Choose a reason for hiding this comment

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

i think it might be a bit confusing to call a Map entries; it's certainly iterable for entries, but that's not exactly the same thing. maybe call it iterableOfEntries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or, should we use [['cat', 'dog'], ['life', 42]]?

Copy link
Member

Choose a reason for hiding this comment

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

That could work too - but it's also nice to show that you can do it with a Map. Maybe both examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will just rename it to iterableOfEntries, too many examples could be confusing?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@xtuc xtuc mentioned this pull request Jan 17, 2019
@littledan
Copy link
Member

These look great! Thanks for contributing. The examples make this page much more descriptive.

Let's see if we can get reviews from the proposal champions. Do you think you could look up who each of them is and @-mention them?

// ↪ 9007199254740991n

const hugeButString = BigInt('9007199254740991');
// ↪ 9007199254740991n
Copy link
Member

Choose a reason for hiding this comment

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

For BigInt, I'd prefer we contrast the Number behavior with the BigInt behavior, and not include a use of the BigInt constructor on a Number (as that could lead people to use a buggy pattern with rounding).

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Hm, I'm not sure what you mean @littledan. I think showing how the constructor works with a Number literal is pretty necessary, in general, but if you mean just for the case of this example, we could definitely use something like this instead:

const x = Number.MAX_SAFE_INTEGER;
// ↪ 9007199254740991, this is 1 less than 2^53

const y = x + 1;
// ↪ 9007199254740992, ok, checks out

const z = x + 2
// ↪ 9007199254740992, wait, that’s the same as above!

const a = BigInt(Number.MAX_SAFE_INTEGER);
// ↪ 9007199254740991n, this is 1 less than 2^53, and also a BigInt

const b = a + 1n;
// ↪ 9007199254740992n, checks out, still a BigInt

const c = a + 2n
// ↪ 9007199254740993n, now that's what I want

Copy link
Member

@littledan littledan Jan 19, 2019

Choose a reason for hiding this comment

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

In general, I am afraid that people could develop false expectations about how the BigInt constructor works, and try to use it with large number literals. I have already gotten a few "bug reports" about this. I am not sure how we could use code samples to avoid the issue; I am not an expert here.

Choose a reason for hiding this comment

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

So I think this with maybe a final line showing it re-rounded? There is nothing bad about showing people how the BigInt constructor is used — not everyone is going to use literals, right?

To some extent, I think we are going to have to accept that people are going to make some mistakes as they get used to new functionality — and then it will become just a known fact about Javascript. To me that's as expected. But then, I also don't get "bug report"s.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just want to emphasize the happy path. Do you see a reason why someone should use the BigInt constructor together with a Number literal? I was imagining that that was subsumed by BigInt literals, but maybe I am missing something.

Copy link

Choose a reason for hiding this comment

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

I could image using the BigInt constructor with a number literal if you were programmatically piping data into some calculations with known BigInts, where the literal wouldn't really work. E.g.

let nums = [4, 10, 17, 22] // pretend these come from some API you do not control
let bigFriend = 9007199254740992n

nums.map((n) => bigFriend + num) // would throw
nums.map((n) => bigFriend + BigInt(num)) // would not

_data/stage3.yml Outdated
example: >
[1, [2, [3]]].flat(Infinity); // [1,2,3]

[2, 3, 4].flatMap((x) => [x, x * 2]); // [[2, 4, 3, 6, 4, 8]
Copy link
Member

Choose a reason for hiding this comment

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

It’d probably be helpful to include a flatMap example that illustrates that it has a depth of 1

Copy link
Member

Choose a reason for hiding this comment

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

This output looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

The output has a syntax error.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was fixed.

{
0: "Hello ",
1: "Hello"
index: 0,
Copy link
Member

Choose a reason for hiding this comment

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

These match objects now also contain a “groups” property (which is undefined if there’s no named captures)

Copy link
Member

Choose a reason for hiding this comment

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

also I’m not sure how to illustrate better that these are not objects, but arrays with string properties

Copy link
Member Author

Choose a reason for hiding this comment

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

//cc @sebmarkbage :-)

Copy link
Member

Choose a reason for hiding this comment

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

I’m the champion of matchAll, and @mathiasbynens was the champion of named capture groups.

Copy link
Member

Choose a reason for hiding this comment

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

also I’m not sure how to illustrate better that these are not objects, but arrays with string properties

I’ve always done it this way, which matches the Node.js REPL:

…
[
	'DEADBEEF',
	index: 19,
	input: 'Magic hex numbers: DEADBEEF CAFE 8BADF00D'
]
…

The only downside is that it’s not valid JavaScript syntax.

@hemanth
Copy link
Member Author

hemanth commented Jan 19, 2019

@littledan Thank you, I had a list of @'s not sure if I can reuse them here.

@hemanth
Copy link
Member Author

hemanth commented Feb 6, 2019

@littledan @ljharb anything else pending on this PR?

I was planning to edit this PR to adhere to issues#66 what say?

@codehag
Copy link
Collaborator

codehag commented Feb 28, 2019

Hey folks, I updated this to remove stage 4 proposals

@codehag
Copy link
Collaborator

codehag commented Feb 28, 2019

This looks good otherwise

@littledan
Copy link
Member

LGTM

@codehag codehag merged commit 26c2073 into tc39:master Feb 28, 2019
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.

6 participants