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.

Sign up for GitHub

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

Jump to bottom

Wrap .toString() #32

Merged
merged 2 commits into from
Jun 13, 2019
Merged

Wrap .toString() #32

merged 2 commits into from
Jun 13, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Jun 12, 2019 •
edited

Fixes #1. Previous PR #6.

This wraps toString() so that:

const wrappedFn = function() {}
const originalFn = function() { return true }
mimicFn(wrappedFn, originalFn)

console.log(wrappedFn.name)
// originalFn

console.log(wrappedFn.toString())
// /* Wrapped with wrappedFn() */
// function() { return true }

The wrapping comment is inserted outside the function body, as opposed to inside. Reasons:

  • I find it easier to read
  • Must easier to implement. Although function.toString() has been recently standardized, there are still differences between engines. Different types of functions are also printed very differently (arrow functions, bound functions, member functions, classes). The body might be on a single line, or not. Those differences makes it harder to insert that comment and be 100% confident every user will get the expected result.
  • Easier to remove for any user trying to parse function.toString() (which by itself is a little of a crazy idea).

If a function is wrapped several times, several comments will be stacked on top of each other, as opposed to using a comma-separated list inside a single comment. This was mostly because it was much easier to implement. I also think it makes the wrapping order more obvious: with a comma-separated list, it's not very clear which wrapper was first used.

@jdalton @tiagosmartinho

ehmicky
ehmicky commented Jun 12, 2019
View reviewed changes
index.js
@@ -29,6 +44,8 @@ const removeProperty = (to, from, property) => {
const shouldCopyProperty = property => property !== 'length';

const mimicFn = (to, from) => {
const {name} = to;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to.name is being modified by the next lines, so we need to keep a reference to the initial value here.

ehmicky
ehmicky commented Jun 12, 2019
View reviewed changes
index.js
const wrappedToString = (withName, fromBody) => `/* Wrapped ${withName}*/\n${fromBody}`;

const changeToString = (to, from, name) => {
const withName = name === '' ? '' : `with ${name.trim()}() `;
Copy link
Collaborator Author

@ehmicky ehmicky Jun 12, 2019 •
edited

Choose a reason for hiding this comment

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

We trim name because function names with trailing whitespaces are not uncommon.

t.is(() => {}).bind().name, 'bound ')

@ehmicky ehmicky mentioned this pull request Jun 12, 2019
Closed
ehmicky
ehmicky commented Jun 12, 2019
View reviewed changes
index.js

const changeToString = (to, from, name) => {
const withName = name === '' ? '' : `with ${name.trim()}() `;
const newToString = wrappedToString.bind(null, withName, from.toString());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We call from.toString() early (during mimicFn()) as opposed to lazily (during to.toString()). This is to ensure from can be garbage collected. Otherwise to.toString() closure would contain a reference to from.

We use bind() instead of a closure for the same reason. I'm not sure whether garbage collectors are keeping references of every variables in the closure scope (changeToString()), or only the ones referenced in the closure function (wrappedToString()), so I chose the safe side.

Also calling from.toString() early allows caching it in case to.toString() is called several times. I'm not sure whether that's already done by V8 under the hood though.

Copy link
Owner

Choose a reason for hiding this comment

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

I think some of this could be useful as code comments, for posterity.

ehmicky
ehmicky commented Jun 12, 2019
View reviewed changes
index.js
const changeToString = (to, from, name) => {
const withName = name === '' ? '' : `with ${name.trim()}() `;
const newToString = wrappedToString.bind(null, withName, from.toString());
Object.defineProperty(newToString, 'name', toStringName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We ensure to.toString.name is still called toString.

ehmicky
ehmicky commented Jun 12, 2019
View reviewed changes
index.js
const withName = name === '' ? '' : `with ${name.trim()}() `;
const newToString = wrappedToString.bind(null, withName, from.toString());
Object.defineProperty(newToString, 'name', toStringName);
Object.defineProperty(to, 'toString', {...toStringDescriptor, value: newToString});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to ensure toString() is not enumerable, otherwise it prints weird in Node.js.

@sindresorhus
Copy link
Owner

Can you mention in the readme that we modify the .toString() representation? Could be useful to know for debugging.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 12, 2019

Fixed.

@ehmicky ehmicky force-pushed the feature/to-string branch 2 times, most recently from fd6e769 to 9075cab Compare June 13, 2019 09:49
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 13, 2019

Rebased.

sindresorhus
sindresorhus requested changes Jun 13, 2019
View reviewed changes
index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Wrap toString() Wrap .toString() Jun 13, 2019
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 13, 2019

Fixed.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 13, 2019 •
edited

CI fails but will succeed after either #36 or #39 is merged.

@ehmicky ehmicky mentioned this pull request Jun 13, 2019
Closed
@sindresorhus
Copy link
Owner

Can you fix the conflict?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 13, 2019 •
edited

Fixed. I've also added a test checking when from has overridden its own toString().

@sindresorhus sindresorhus merged commit 39f96c4 into master Jun 13, 2019
@sindresorhus sindresorhus deleted the feature/to-string branch June 13, 2019 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Function toString?
2 participants
@ehmicky @sindresorhus