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

Single-layer property replacement is inappropriate for ES modules #262

Closed
searls opened this issue Jun 14, 2017 · 2 comments · Fixed by #268
Closed

Single-layer property replacement is inappropriate for ES modules #262

searls opened this issue Jun 14, 2017 · 2 comments · Fixed by #268

Comments

@searls
Copy link
Member

searls commented Jun 14, 2017

RIght now, td.replace'd CJS modules enjoy a single-layer of additional properties (say, a function with other static functions as properties) being faked along with the top-level export.

This is incongruent with ES modules, because that first layer of replacements typically is just enough to cover the default property on the exported object (which is how transpilers translate those imports to CJS), which means the same module will be faked differently if all a person does is convert it a CJS module to an ES module.

Going forward I think we need to do one of two things:

  1. Pick a depth and then make it feel consistent for both module types (e.g. perhaps we keep it as is, but add an extra layer of function fakes and property cloning for any export named default
  2. Recursively fake whatever we find on a module, and do whatever work we need to break out of cyclic structures

Option 2 is probably more appropriate for simplicity & consistency sake, but it also runs the risk of encouraging folks to return deeply-nested mocks, which is separately a little worrying. That said, even that would be better than the current state: accidentally returning people partial mocks that have some properties that are still real (or missing)

@searls
Copy link
Member Author

searls commented Jun 18, 2017

Here's some pseudocode for what I'm thinking. This'll be a new freshly-driven src/imitate module which will recursively iterate properties of an original item and copy and/or fake them to a target, while also doing something to break cycles.

If we do it right, it will:

  • actually detect cycles and fast-return any previously encountered objects (tricky because you might be looking at an imitation of a thing so we need a way to track them back to the original
  • not inadvertently overwrite deeply-nested properties in such a way that they are affected on the original (e.g. be careful about reaching deep into a shallow clone and monkeying around
  • allow us to rid all references to the td.replace throws with objects created with Object.create #257 modules gatherProps, copyProps and filterFunctions from all call sites in the "legacy" codebase, effectively getting td.func/td.object/td.constructor out of the business of imitating props at all.
  • give us a better sense of what the actual difference is between td.func and td.constructor, since the recursive algo will of course hit any function's prototype and iterate that too, making the distinction as we've expressed it up to now almost totally moot (for instance: why aren't constructor functions themselves test doubles in that case? must they actually be POJSF's wrapped with another fake? I'm starting to doubt it

Psuedocomments

export default (original, target, encounteredObjects = []) => {
  // 1. function, object, and constructor all immediately bail out to imitiate after creating their top-level thing (func, empty obj, func, respectively)
  // 2. guard: return original if not isObjectLike or isFunction
  // 2a. guard: return original if already encounteredObjects includes it
  // 3. gather all props on original
  // 4. copy all props to target
  // 5. for each prop:
  //   - if isConstructor, call constructor()
  //   - else if function call function()
  //   - else empty {}
  //   - call self(prop, +that new thing, encounteredObjects.concat(original)
}

@searls
Copy link
Member Author

searls commented Jun 24, 2017

Landed in 3.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant