[WIP] js: tighten up internalize and externalize #618

myitcv · 2017-04-06T09:16:08Z

!! DO NOT REVIEW !!

NEEDS REBASE AFTER #687 IS MERGED

Status

Step 1 and 2 (see below) review.

Description

A work-in-progress PR to complement the discussion in #617

Tighten up $internalize and $externalize such that:

zero val translation of undefined and null is well defined
the domain of valid values for a given target type is well defined
runtime errors are thrown for invalid values

See impact of this PR on honnef.co/go/js/dom in this comparison

Documentation

Proposed human-readable definitions of $internalize and $externalize can be seen here.

Scope of `$internalize` testing

The domain of source values we need to consider is:

null
undefined (there is no need to distinguish between a field/variable being explicitly assigned the value of undefined and it being so by virtue of not having been initialised)
Primitive and non-primitive values (see MDN)

Overall PR TODO list

Agree on structure of code, tests and docs for a couple of types, e.g. string and bool
Work out and fix reflection (manifests via TestReflection failing in js tests)
Complete revised implementation of $internalize and $externalize for all types to allow for testing that code in real-life code bases (not just tests)
Finish docs for all types
Tighten up wording of *js.Object-special struct types in js package (unless we work this into the tables for $internalize and $externalize
Document ways in which you can fallback to Javascript "loose" type conversions

Consideration for later PRs

Splitting $internalize and $externalize into separate, smaller type-specific functions
Fixing the fact that null and undefined do not internalize as (*T)(nil) for a *js.Object-special struct type T

dmitshur · 2017-04-08T04:39:47Z

js/js_test.go

+	type S struct {
+		*js.Object
+
+		string `js:"string"`


Since this is a test of internalization logic (as I understand from our conversation in #617), I would suggest rewriting it to avoid using js struct field tags. I think it's simpler to use Object.String method, it's lower level and more explicit, making it more clear what the test is testing.

@shurcooL I think for completeness we'll need to test String() as well, but in order that we can cover all Go types I think we need to follow the struct field tags approach... unless I'm missing something?

but in order that we can cover all Go types I think we need to follow the struct field tags approach...

Are methods like String, Float, Bool Uint64, etc., not sufficient? What's not possible without struct field tags?

All the types that do not have corresponding methods on Object. e.g. the specific numeric types like int8.

Can Interface().(int8) be done for those, or is that not equivalent?

I think Interface() is another test case altogether because it uses the Javascript type of the value to decide which Go type to resolve to. So in the context of this discussion, I suspect use of Interface() wouldn't actually be a problem per se.

What you've proposed transpiles to:

$assertType($internalize(o.A, $emptyInterface), $Int8)

So we still need to have test cases for all the Go types... because it's $internalize that we're ultimately testing here.

I see, thanks. Then it's okay to use the struct field tags, but my request is to document that what's being tested is internalize behavior, not the struct field tag behavior.

Ah I see; 100% agree on that point.

flimzy · 2017-07-29T21:22:57Z

js/js.go

+//
+// When values pass from Go to Javascript, a process known as externalization,
+// the following conversion table is applied:
+//


Table missing?

See next line.

flimzy · 2017-07-29T21:23:29Z

js/js.go

+//
+// Externalization
+//
+// When values pass from Go to Javascript, a process known as externalization,


Incomplete sentence

Thanks @flimzy - I should have mentioned this entire PR is still very much WIP.

In particular, I haven't even started on the $externalize side of things. Had some fairly interesting edge cases on the $internalize side of things

myitcv · 2017-07-30T20:35:43Z

@flimzy @shurcooL - thanks both for taking a look at this.

I should have been clearer in my update message in #617 because I value your time... This is still WIP so I will give you a shout when it's essentially "ready" for review. There are still too many rough edges.

My plan is to get a couple of types (probably string and bool) done for both $internalize and $externalize and then get some feedback on how things look, from code and tests through to docs etc.

Just wanted to post an update to confirm I'm still "on" this, albeit after a significant period of inactivity!

dmitshur · 2017-07-30T20:37:52Z

I should have been clearer in my update message in #617 because I value your time... This is still WIP so I will give you a shout when it's essentially "ready" for review. There are still too many rough edges.

No problem.

I suggest using Go code review convention for achieving this, put a "DO NOT REVIEW" text anywhere in the PR description to signal that. (See https://groups.google.com/d/msg/golang-dev/YU56eKYiXJg/K1CkXo7Iy0gJ.)

myitcv · 2017-07-30T20:54:21Z

I suggest...

Done. I've switched from using my [WIP] convention in the title to your suggestion in the PR description.

Thanks again

myitcv · 2017-08-22T13:16:56Z

@shurcooL @flimzy et al.

Given there's quite a lot to work through in this PR, I've updated the description in this PR with, amongst other things, the following:

Status
Overall PR TODO list

Would welcome your thoughts on the TODO list, in terms of content but also approach (i.e. using the TODO list as a means of communicating how I plan to go about things).

At this point I think I've got to the end of step 1, hence am marking this for a review.

All thoughts/feedback welcomed.

Thanks

myitcv · 2017-08-22T13:23:08Z

Worth pointing out that at this stage, not least because of point 2 from the TODO list, I'm not expecting the build to pass.

flimzy · 2017-08-22T14:18:20Z

js/js.go

+//  | bool           | none          | null                    | false  |
+//  |                |               | undefined               | false  |
+//  |                |               | false †                 | false  |
+//  |                |               | new Boolean(false) †    | false  |


Is it worth including true examples here, for the sake of completeness, similar to the non-zero value examples for strings?

We would end up adding two lines: true and new Boolean(true) - hence why I opted for the † callout.

flimzy · 2017-08-22T14:21:02Z

js/js_test.go

@@ -467,7 +467,7 @@ func TestCallWithNull(t *testing.T) {
 func TestReflection(t *testing.T) {
 	o := js.Global.Call("eval", "({ answer: 42 })")
 	if reflect.ValueOf(o).Interface().(*js.Object) != o {
-		t.Fail()
+		t.Fatal()


AFAIK, Fatal() is for reporting a failure message. Maybe FailNow() would be more semantically correct?

Is it not possible to include a helpful error message in the t.Fatal call? If possible, that's better.

To be honest I was being a little lazy here in just flipping the existing t.Fail() to t.Fatal() to help with debugging step 2 from the TODO.

I'm sure we could add helpful messages to the t.Fatal() calls... because the existing t.Fail() calls are equally lossy. Could even be something we do in another PR.

It's usually easier to do these minor things as you're working in a relevant context than later. But this is completely up to you, and not critical for getting the PR into a mergeable state.

flimzy · 2017-08-22T14:24:22Z

js/js_test.go

+	fieldName := "string"
+	zero := ""
+
+	s := &Internalize{Object: js.Global.Get("Object").New()}


Stylistic note: I wonder if a table-based test would make sense here? It may make the tests more readable, reduce the need for the // *** comment blocks, and if coupled with t.Run(...) calls for each test, could make the output more readable.

You raise a good point. I half considered this myself, but postponed giving it much thought until we've confirmed we're actually testing the right stuff... at which point extracting to table tests should be relatively easy.

The little thought I did give it however amount to the following: I'm not convinced we can actually use table tests. We need to address specific fields of the Internalize struct and do so in such a way that the gopherjs compiler sees us doing so and compiles to the corresponding Javascript. Put another way, the code we write in these tests needs to correspond exactly to the code that will be written in the wild.

Therefore the one approach I have considered fairly seriously is code generating the tests. Because they do follow a formula that's for sure.

But as I said, I haven't given this much time/though so would very much welcome any suggestions.

@flimzy just pushed up a code-generated version of the tests which seems to work well. Let me know what you think.

myitcv · 2017-08-25T09:38:52Z

Just pushed up a new commit per #617 (comment)

flimzy · 2021-12-28T12:47:55Z

Nearly 4 years later, @nevkontakte and I have decided to close this PR. We like the effort here, and some of it will be very informative if we address these issues in the future, but with limited bandwidth, and the author having left GopherJS development, we feel this PR is at a dead end.

myitcv changed the title ~~[WIP] fixes for internalize and zero val translation of undefined and null~~ [WIP] Tighten up internalize for zero val translation of undefined and null Apr 6, 2017

myitcv force-pushed the internalize_zero_val branch from 0bbcb74 to a8c3aed Compare April 7, 2017 12:20

dmitshur reviewed Apr 8, 2017

View reviewed changes

myitcv force-pushed the internalize_zero_val branch from a8c3aed to 1b130f5 Compare April 8, 2017 16:39

myitcv changed the title ~~[WIP] Tighten up internalize for zero val translation of undefined and null~~ [WIP] Tighten up internalize Apr 8, 2017

myitcv force-pushed the internalize_zero_val branch from 1b130f5 to 0d5cce0 Compare April 8, 2017 16:45

myitcv mentioned this pull request Apr 8, 2017

Proposal: tightening up $internalize and $externalize (for zero values) #617

Open

myitcv force-pushed the internalize_zero_val branch 3 times, most recently from d23fda2 to 93b99d9 Compare July 29, 2017 18:29

myitcv changed the title ~~[WIP] Tighten up internalize~~ [WIP] Tighten up internalize and externalize Jul 29, 2017

flimzy reviewed Jul 29, 2017

View reviewed changes

myitcv changed the title ~~[WIP] Tighten up internalize and externalize~~ Tighten up internalize and externalize Jul 30, 2017

myitcv force-pushed the internalize_zero_val branch 3 times, most recently from f9d8435 to 517a713 Compare August 22, 2017 13:07

flimzy reviewed Aug 22, 2017

View reviewed changes

myitcv force-pushed the internalize_zero_val branch 5 times, most recently from 0a359b9 to 19bf589 Compare August 27, 2017 22:28

myitcv changed the title ~~Tighten up internalize and externalize~~ js: tighten up internalize and externalize Apr 2, 2018

WIP fixes for internalize and zero val translation of undefined and null

ab5ceb8

myitcv force-pushed the internalize_zero_val branch from dabda69 to ab5ceb8 Compare April 20, 2018 11:25

myitcv changed the title ~~js: tighten up internalize and externalize~~ [WIP] js: tighten up internalize and externalize Apr 20, 2018

myitcv mentioned this pull request Apr 23, 2018

vfsgendev updates modification times #804

Open

flimzy closed this Dec 28, 2021

[WIP] js: tighten up internalize and externalize #618

[WIP] js: tighten up internalize and externalize #618

Uh oh!

Conversation

myitcv commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

Description

Documentation

Scope of $internalize testing

Overall PR TODO list

Consideration for later PRs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myitcv commented Jul 30, 2017

Uh oh!

dmitshur commented Jul 30, 2017

Uh oh!

myitcv commented Jul 30, 2017

Uh oh!

myitcv commented Aug 22, 2017

Uh oh!

myitcv commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myitcv commented Aug 25, 2017

Uh oh!

flimzy commented Dec 28, 2021

Uh oh!

Uh oh!

myitcv commented Apr 6, 2017 •

edited

Loading

Scope of `$internalize` testing

dmitshur Apr 8, 2017 •

edited

Loading