Skip to content

Conversation

blakeembrey
Copy link
Owner

Fixes the issues from #16.

@blakeembrey blakeembrey force-pushed the be/fix-method-definitions branch from 9c92dcd to 043810a Compare February 5, 2019 06:55
@coveralls
Copy link

coveralls commented Feb 5, 2019

Coverage Status

Coverage decreased (-3.8%) to 96.203% when pulling f180068 on be/fix-method-definitions into f06123e on master.

if (!isMethodDefinition(fn)) return value;

var name = isValidVariableName(fn.name) ? fn.name : '';
return prefix + ' ' + name + value.replace(/^[^(]+/g, '');
Copy link
Contributor

@rhendric rhendric Feb 5, 2019

Choose a reason for hiding this comment

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

This isn't quite right; the method name could have a ( in it.

Test with:

          it('should stringify extracted methods with tricky names', function () {
            var fn = eval('({ "a(a"(x) { return x + 1; } })')['a(a'];
            expect(stringify(fn)).to.equal('function (x) { return x + 1; }');
          });

[Edit: the "a(a" name can't appear in the output, since it's not valid JavaScript to have a name like that in a non-method function. So the right thing to do is strip it. What a rabbit hole...]

Copy link
Owner Author

Choose a reason for hiding this comment

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

@rhendric That's a good catch. I guess this regexp needs to get more complicated here 😦 I don't know if I can make everything pass on both node.js v4 and v6 since they serialize differently and with different whitespace based on how it was defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new toString logic (which I think started with node.js 10?) is a real pain to handle 100% correctly; the worst case is methods with computed properties, whose toStrings contain the definition of the computed property rather than the value, square brackets and all. Properly stripping those—and they do need to be stripped, lest they reference variables not present in the scope when the stringified object is being eval'd—requires some kind of parser, not just a regexp, since computed properties can contain arbitrary JavaScript.

Those problems notwithstanding, I started today on a new implementation that supports all extant node.js versions (and SpiderMonkey, which naturally does its own thing), which I should have ready for you to review by the end of the week. It... might be more code than either of us would like, but once it's done at least we can talk about exactly how much more code, and whether you think the corner cases it handles are worth it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe we can get away with some light weight parsing? E.g. quote, escape and bracket matching only. It'd be <10 lines since we don't care about the expression itself, then always rebuild into function syntax using function.name. Have isMethodDefinition return an index for whether it's a match (e.g. where to strip up to or -1 if not a match). I'll check out your implementation but know you don't need to go too crazy to make this work. Computed properties are an extremely good catch, it wouldn't have occurred to me - need to add a lot of cases now the toString() is purely the code expression.

@blakeembrey blakeembrey closed this Mar 3, 2019
@blakeembrey blakeembrey deleted the be/fix-method-definitions branch April 14, 2021 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants