andrewplummer / Sugar

A Javascript library for working with native objects.
https://sugarjs.com/
MIT License
4.53k stars 306 forks source link

Can't use sugar RegExp on String#match #611

Open alFReD-NSH opened 7 years ago

alFReD-NSH commented 7 years ago

Here's what I get when I try:

> ''.match(new (require('sugar').RegExp))
TypeError: Method RegExp.prototype.toString called on incompatible receiver undefined
    at toString (<anonymous>)
    at SugarChainable.toString (/home/farid/repos/hubot-bamboohr-timeoff/node_modules/sugar-core/sugar-core.js:755:38)
    at String.match (native)
    at repl:1:4
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:440:10)
    at emitOne (events.js:120:20)
> ''.match(new RegExp)
[ '', index: 0, input: '' ]

I believe it happens because string#match() doesn't recognise Sugar RegExp as a proper RegExp, so it converts it to string. On a related note, I believe Sugar also don't recognise it's own RegExp in internal methods:

> Sugar.Object.find({ test: 'test' }, /test/)
'test'
> Sugar.Object.find({ test: 'test' }, Sugar.RegExp('test'))
undefined
> Sugar.Object.find({ test: 'test' }, new Sugar.RegExp('test'))
undefined
andrewplummer commented 7 years ago

That is correct, even though a Sugar.RegExp instance (aka a "chainable") has native methods mapped onto it, they are not the exact same instance methods as on native RegExp, but instead forward the wrapped regex object to the native methods. This means that Sugar.RegExp instances (as well as instances of all Sugar.XXX classes) should not be considered compatible with native methods in such a way as they can be thrown around as if they were their respective native instances. The reason for this is that Javascript does special checks internally and is not "duck typed" when it comes to natives, restricting proxied classes, so this cannot be avoided.

andrewplummer commented 6 years ago

So this has taken some time to look into... there are a couple issues happening here at the same time...

If you want the short version, I'm not sure why I didn't mention this before but you need to unwrap the chainable with the raw property... so 'foo'.match(sugarRegExp.raw). I would love it if Sugar chainables could be tossed around like native objects but that doesn't seem to be possible.

Now for the details why:

It seems that String.prototype.match does an initial check on the passed in argument to see if it is a RegExp. If it isn't, it performs a coercion. So if you pass a string in ('f'.match('f')), you are actually matching against /f/. The method does not recognize a Sugar chainable object as a true RegExp, so it calls its toString method to perform a coercion. Now already we have an issue because the toString method wraps the native method, so you would get "/f/" as the result and now you are basically matching the string on /\/f\// instead, which would fail. However, Sugar also wraps chainable methods with another chainable (so they can chain) so toString returns a non-string object and the internal match method is failing on that.

The 2nd issue is possible to overcome by having toString return a primitive string, however as mentioned that string would be wrong... patching toString to return the regex source instead of the regex itself would kind of work, but then the regex flags would fail and the whole thing would be wonky... also you would get a different result for reg.toString() and sugarReg.toString() which is also wonky.

There simply is no point at which Sugar can get in and unwrap itself to native methods once it's passed into match. This unfortunately means that the policy has to be that if you are passing sugar objects into native methods, you have to unwrap them first with raw. This should already be in the docs, but it helps to describe here one more reason for this.

The only thing that can be done here is make the API friendly enough so that it's obvious how and when this needs to be done. I'll be thinking about it more but where we're at now might be as good as we can do....

pepkin88 commented 3 years ago

@andrewplummer

There simply is no point at which Sugar can get in and unwrap itself to native methods once it's passed into match.

There actually is a way. You can pass any object to String.prototype.match as long as it implements [Symbol.match] method. Example:

'foo'.match({
    re: /o+/,
    [Symbol.match](s) {
        return this.re[Symbol.match](s)
    },
})

it outputs the same result as 'foo'.match(/o+/) does.

More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/@@match https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/match