imitate / replace things deeply (instead of shallow) #268

searls · 2017-06-23T01:54:49Z

Build on #257 by creating a single unit responsible for all "imitation" of real things, be they plain functions, objects, or constructors.

This does a few neat things for the library in terms of enhancement:

It seems to fix the instanceof/inheritance issues that forced the 3.0 major bump. If this turns out to be true, it means the config property can be eradicated and we can put that whole mess behind us
It will perform an arbitrarily deep imitation of whatever you pass it, cloning most things and replacing functions with test doubles. This will make it more suitable for ES modules
It will remove the inconsistencies between td.func/td.object/td.constructor, in fact, most of the implementation of each of those can be thrown out
Work fine when passed things containing cyclic references

This is an incremental improvement that shook out of the Big Epic Rewrite in #263

Concerns

If you are in the habit of calling td.replace on massive modules (with hundreds or thousands of cumulative properties), this is going to be realllllllly slow compared to what you're used to, at potentially no added benefit. Trade-offs here:

The new implementation could surely stand to be optimized, since it's new,
You really shouldn't be replacing massive dependencies in a unit test, because (a) you shouldn't be writing massive dependencies, and (b) you shouldn't be mocking what you don't own (QED)
I'm open to the idea that a maxImitationDepth config setting may be appropriate (note that the current implementation successfully breaks cycles, but will otherwise recurse to arbitrary depth)

Todo

I made a bit of a mess in the process of getting this over the hump and the build passing. Some to-dos remain:

Fixes #262

This was causing all our metaprogramming that checked types to break. See: nodejs/node#13827

This is necessary for avoiding cases where even accessing the property will set off a getter, or blow up in strict mode (e.g. accessing arguments or caller/callee on something). Instead, we'll look at the descriptor's `value` property, which is relatively "inert" by comparison

When copying props, you can adjust the value with a func arg

Does not work but figured like I may as well save my place

Should overwrite fields, but only if they're both writable & configurable. If the target has a field that's somehow neither of these, then it's almost certainly some kind of built-in like Function.prototype.length

It no longer needs the original because it has the full property descriptor

This required using the most-valuey of types as base cases in the recursion, because it'd muck with the perceived value of things like Date and moreover would fake 30+ string methods for probably zero benefit and lots of confusion. (Only affects boxed instances of these prims)

Surprisingly lodash isEqual fails on two errors but only if one is cloned. Meanwhile, assert module will pass deepEqual on two errors if the messages differ. Fantastic

Push all the naming chains into the imitation object, since it's the only call site with sufficient context

This got the tests passing, but may save us the need for extending types options (thanks to just using prototype and not class syntax)

Add a Map polyfill to get things working under older browsers, also refer to Symbol in such a way that it's backwards compatible

I did this in an example project to be sure it was a native (not transpiled) class being "extended" by td.js. This is the case that was failing during the 2.x series and necessitated the extendCo… option for 3.x This test passes, so we can delete that option next

The improvements in the new imitate function sidestep the babel class extension issue that prompted the introduction of this option. All fakes will pass instanceof checks as a matter of course now. Of course, this means that a major bump will be necessary, since that config option will have been removed and anyone setting it will blow up…

I feel like I earn 3 points for lazily giving people a let-me-github-that-for-you URL and lose 2 points for lazily pasting in an error message

Doing this in prep of adding a second teenytest suite under test/safe

Also we stopped printing '#' as shorthand for 'prototype'. It was complicating the code, and on further reflection it makes the messages harder for anyone who's not "in the know" with respect to '#' being a symbol for "instance method" (as opposed to '.' for plain function)

This refactor started by teasing the tangled function into its separate roles, then pushing the behavior down into 3 new units. The unit test for the refactored thing is particularly interesting, because it required use of argument captors to trigger the downstream behavior. This was necessary because the method was recursive, and we needed to invoke the subject twice, once indirectly. It's hard to find good examples of argument captors, so here you go.

jasonkarns

Same comment, just repeated everywhere: naming the default export to provide a useful function name when debugging/stack-trace

I'm excited that this reduces the complexity of the imitator logic!!

jasonkarns · 2017-06-24T01:38:11Z

src/constructor.js

-import copyProps from './share/copy-props'
-import gatherProps from './share/gather-props'
-import filterFunctions from './share/filter-functions'
+import imitate from './imitate'

 export default (typeOrNames) =>


I'd suggest naming the function so you'll get more useful stack.

jasonkarns · 2017-06-24T01:38:29Z

src/function.js

-import gatherProps from './share/gather-props'
-import copyProps from './share/copy-props'
-import filterFunctions from './share/filter-functions'
+import imitate from './imitate'

 export default (nameOrFunc, __optionalName) =>


same here (name the exported func)

jasonkarns · 2017-06-24T01:38:40Z

src/function/index.js

@@ -1,14 +1,12 @@
 import _ from '../wrap/lodash'

 import create from './create'
-import imitate from './imitate'
+import imitate from '../imitate'
 import remember from './remember'

 export default (nameOrFunc) => {


jasonkarns · 2017-06-24T01:38:51Z

src/imitate/copy-props.js

@@ -0,0 +1,31 @@
+import _ from '../wrap/lodash'
+
+export default (target, props, visitor) => {


more anon default exports...

jasonkarns · 2017-06-24T01:39:05Z

src/imitate/create-imitation.js

+
+import tdFunction from '../function'
+
+export default (original, names) => {


jasonkarns · 2017-06-24T01:39:16Z

src/imitate/initialize-names.js

@@ -0,0 +1,16 @@
+import _ from '../wrap/lodash'
+
+export default (original, names) => {


jasonkarns · 2017-06-24T01:39:22Z

src/imitate/overwrite-children.js

+import gatherProps from './gather-props'
+import copyProps from './copy-props'
+
+export default (original, target, overwriteChild) => {


jasonkarns · 2017-06-24T01:39:36Z

src/replace/module.js

@@ -7,11 +9,16 @@ quibble.ignoreCallsFromThisFile()
 export default function (path, stub) {


searls · 2017-06-24T01:45:48Z

lol @jasonkarns why did you leave the same comment a million times.

FWIW, I tend not to name these functions since the stack trace gives me the file name & I tend to rename really aggressively, and naming the function adds yet another thing I have to change (in addition to file name, test file name, the test's import, and any production imports). Because I tend to create a lot of small things and change them pretty frequently, I tend not to value naming functions like this until well after the dust has settled on the design

jasonkarns · 2017-06-24T01:55:11Z

Was just tracking all the spots so nobody else had to search :)

since the stack trace gives me the file name

Didn't realize babel did this now (always?). Hoping it works for the browserified version. 🤷‍♂️

Turns out most of the logic was pretty dumb and it's new behavior so I just axed most of it

searls · 2017-06-24T02:04:47Z

Re: babel, it depends on how you compile it. We compile via a filter operation into lib (meaning no concatenation occurs for node users) and we separately babelify for browser users. The former gives the pretty stack traces. The latter, well, I'll welcome a fixup PR some time after this big epic rewrite to sprinkle in all the right names

These are just the node-level replacement, their properties (or for arrays, elements) will be replaced by overwriteChildren Simplied somewhat, when I learned through the test that underscore's clone works well enough for primitives (one would trust, efficiently enough)

Lots of complexity got swept under this rug, really causing some pain at the periphery. Since it's a leaf node, will I ever look at it again? Probably not. Some notes: * ../../../.. is outrageous. I should figure out how to go about getting the unit tests colocated with the source soon. * 5 arguments for chain-prototype.js? Seriously? That can't be necessary * This overwrite-children/index test is really the kitchen sink of edge cases for delegator objects. It has replaced deps, it has one-off test double functions passed as arguments, it has argument captors for getting callbacks under isolated tests, it has verifications that certain dependencies were absolutely not called. It's got it all! All of these are smells that this unit is weird/awkward. It's possible that things got complex because the code originated while trying to tidy up the behavior originally in the context of a big method with lots of closure-scoped variables available. That explains the 5 arg function (once extracted and closure-scope is lost), as well as the visitor pattern sort of callbacks to keep the recursion step out-of-sight of the extracted unit. Unsure how I'd rework it at this point to breathe like a normal function, though. That's why I should have test-drove this from scratch instead of test-aftering and pushing down all these things! Whoops!

searls · 2017-06-24T13:37:57Z

Landed in 3.2.0

searls added 29 commits June 20, 2017 20:06

Fix the repl by using useGlobal

c6c3ff0

This was causing all our metaprogramming that checked types to break. See: nodejs/node#13827

Avoid copying & then mutating by accepting a visit

c451e72

When copying props, you can adjust the value with a func arg

Rough draft of this imitation func

3f7c935

Does not work but figured like I may as well save my place

Change rules on copyProps

a86905a

Should overwrite fields, but only if they're both writable & configurable. If the target has a field that's somehow neither of these, then it's almost certainly some kind of built-in like Function.prototype.length

Reduce args on copyProps

37ca57c

It no longer needs the original because it has the full property descriptor

Test error objects specially

8469b45

Surprisingly lodash isEqual fails on two errors but only if one is cloned. Meanwhile, assert module will pass deepEqual on two errors if the messages differ. Fantastic

standard --fix

b4f6eea

Get more serious about improving naming.

11d8dfd

Push all the naming chains into the imitation object, since it's the only call site with sufficient context

Wire up new imitation center.

d83f661

This got the tests passing, but may save us the need for extending types options (thanks to just using prototype and not class syntax)

standard --fix

dd437a2

Get this working under phantom (Map, Symbol)

14d62b1

Add a Map polyfill to get things working under older browsers, also refer to Symbol in such a way that it's backwards compatible

All of this is obviated by src/imitate

b5e952c

Now that imitate is central, these aren't shared

1a48ede

Increase timeout b/c SLOOOOOOW'

519cd58

Remove unused lodash functions from wrapper

0f66b8e

Spare ourselves the major bump by deprecating prop

55d58fe

I feel like I earn 3 points for lazily giving people a let-me-github-that-for-you URL and lose 2 points for lazily pasting in an error message

standard --fix

7ef2b5d

Move old test suite to regression/

5f15650

Move typescript too. test/ is just teenytest

3fd25b5

Move unit tests under test/unit

d3b09fb

Doing this in prep of adding a second teenytest suite under test/safe

Move imitate fun

fbb1fbf

Use test/safe instead for call-through test

1b2c936

Fix test for Node 4 (requires strict mode)

9aded7b

jasonkarns reviewed Jun 24, 2017

View reviewed changes

Drive out a fresh impl of initialize-names

37ccdc8

Turns out most of the logic was pretty dumb and it's new behavior so I just axed most of it

searls added 7 commits June 23, 2017 22:28

standard fix

92f1354

Add a silly deeply nested example

754fd79

standard --fix

9d7fb8d

Clear out needless indirection

76093c1

Test the primitive-checker

2fce02b

searls merged commit 2fce02b into master Jun 24, 2017

searls deployed to github-pages June 24, 2017 13:34 View deployment

searls mentioned this pull request Jun 24, 2017

td.constructor() breaks instanceof #267

Closed

searls deleted the spike-imitation-object branch June 24, 2017 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imitate / replace things deeply (instead of shallow) #268

imitate / replace things deeply (instead of shallow) #268

searls commented Jun 23, 2017 •

edited

Loading

jasonkarns left a comment •

edited

Loading

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

jasonkarns Jun 24, 2017

searls commented Jun 24, 2017

jasonkarns commented Jun 24, 2017

searls commented Jun 24, 2017

searls commented Jun 24, 2017

		@@ -0,0 +1,31 @@
		import _ from '../wrap/lodash'

		export default (target, props, visitor) => {


		import tdFunction from '../function'

		export default (original, names) => {

		@@ -0,0 +1,16 @@
		import _ from '../wrap/lodash'

		export default (original, names) => {

		@@ -7,11 +9,16 @@ quibble.ignoreCallsFromThisFile()
		export default function (path, stub) {

imitate / replace things deeply (instead of shallow) #268

imitate / replace things deeply (instead of shallow) #268

Conversation

searls commented Jun 23, 2017 • edited Loading

Concerns

Todo

jasonkarns left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

searls commented Jun 24, 2017

jasonkarns commented Jun 24, 2017

searls commented Jun 24, 2017

searls commented Jun 24, 2017

searls commented Jun 23, 2017 •

edited

Loading

jasonkarns left a comment •

edited

Loading