sinonjs / sinonjs/sinon

Simplify/make props definition consistent

Open
#2,238 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Difficulty: Medium hacktoberfest Help wanted Improvement pinned
Dominant language
JavaScript
Stars
9.8k
Forks
809
PR merge metrics
No merged PRs in 30d

Description

How we handle defining props (basically how we replace props with fakes or stubs) is handled a bit nilly-willy, causing random bugs. An analysis in #2237 showed that any special handling only needs to happen when two things happen: the prop is non-configurable and it is owned by the object.

This issue exists to remind us that we should devise a way to handle this and then hunt down which of the ten-or-so places that are applicable.

This is a section from #2237 that contains details about this issue. Refer to that case for background info:


... there are several things I found out while researching this that needs fixing.

Why do we not always set configurable: true?
Looking at the git log I found #1456 and #1462 which basically changed true to use false if that attribute was already set to false. Why does that make a difference? Well, as long as the prop is writable Object.defineProperty will only throw if configurable is false AND any of the other fields change their value. So we can do Object.defineProperty(obj, prop, {value:42}) on a non-configurable prop, as the other fields will be left alone.

Why do we tread enumerable differently than configurable?
Given the above, reasoning, why do we always set enumerable to true? Should it now be subjected to the same handling as configurable? Yes it should have been treated in the same manner and in fact, trying to do stub.value(42) on a prop that is neither configurable nor enumerable will throw in Sinon today. As should be apparent from the above section, this does not need to happen! We only need to keep properties the same, if they exist, otherwise, use sensible (permissive) default values.

Given that, I think we need to create a separate issue to cover that. A basic fix is to make our own defineProps that does this:

var getObjectDescriptor = require('./get-object-descriptor');
var defaultPropValues = Object.freeze({configurable: true, enumerable: true, writable: true});

// reuse existing values, if they exist
function defineProperty(obj, prop, newAttributes){
    var originalDescriptor = getObjectDescriptor(obj, prop);
    var descriptor = Object.assign({}, defaultPropValues, originalDescriptor || {}, newAttributes);
    Object.defineProperty(object, prop, descriptor);
}

The reason we need to merge in defaultProps is that all of the listed ones are false by default when using Object.defineProperty while we usually want them to be true (like they are in normal assignment operations).


The suggested fix is just a quick suggestion. Another might be:
Always create props with the default permissive settings, unless propDefinition.isOwn && propDefinition.configurable === false, in which case you need to merge them as done in the suggestion.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the background in issue #2237, then trace the roughly ten prop-definition sites that replace props with fakes or stubs. Consolidate their handling so owned, non-configurable properties preserve required attributes while other properties use permissive defaults, and verify consistent behavior across those sites.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.