jshttp / etag

Create simple HTTP ETags
MIT License
256 stars 32 forks source link

fs.stat is not an object #10

Closed obastemur closed 9 years ago

obastemur commented 9 years ago

from the index.js

function isstats(obj) {
  // not even an object
  if (obj === null || typeof obj !== 'object') {
    return false
  }

Above check fails for SpiderMonkey since it returns 'function' for new function instances. I don't know exactly how this check works. Why do we need the typeof stat is 'object' ?

dougwilson commented 9 years ago

Unfortunately this is a Node.js module. Can you point me to the ECMA specification that shows the typeof behavior you are referring to?

dougwilson commented 9 years ago

Using the Firefox web browser, I'm also seeing object and not function. Is Firefox also wrong?

> typeof new (function Stats(){})
"object"
obastemur commented 9 years ago

Unfortunately this is a Node.js module.

We are trying to use this on JXcore SpiderMonkey build. It's okay for V8 build but V8 doesn't work everywhere.

'Can you point me to the ECMA specification that shows the typeof behavior you are referring to?'

EcmaScript spec. says return 'function' if the object has 'call'. This 'Stat' thing is actually a native function instance (from node_file.cc / .h) V8 doesn't put 'call' on new instance. SpiderMonkey does. They both behave properly when it comes to EcmaScript spec. but how they initialize the new native function is another thing.

obastemur commented 9 years ago

Unfortunately 'new function' call on the JS land is a bit different comparing to 'native side'. Stat is a native instance of a function.

dougwilson commented 9 years ago

I can make the change if I can add JXcore to our CI. Please provide instructions and I can properly supprot JXcore without hesitation.

obastemur commented 9 years ago

My initial point was, why it can't be 'callable' ? if you call instanceof Stats, it will return true whether it's function or object.

obastemur commented 9 years ago

'I can make the change if I can add JXcore to our CI. Please provide instructions and I can properly supprot JXcore without hesitation.'

Thanks!

dougwilson commented 9 years ago

I have referenced ECMA-262 in the meantime and in section 11.4.3, I see the table shows that I should accept function. This means I'll make this change and issue a bug fix. I would be, as the maintainer of Express JS and all sub modules, completely willing the fully support JXcore as a first-class citizen of Express. As such, to be a first-class citizen, I do need a way to run a CI on tests for them :) So I would like to know how :)

obastemur commented 9 years ago

Sounds great and Thanks again! Travis-CI right? We didn't release SM build yet. Once we release it, perhaps it will be a candidate for testing SpiderMonkey JavaScript on Travis-CI. At that point it won't be much tricky to run though.

I will keep you updated on this.

dougwilson commented 9 years ago

Cool, keep me posted :) Travis CI is preferable, but any (free) CI service is also fine if it ends up being a different one ;

dougwilson commented 9 years ago

Basically, I'm trying to figure out what the checks I need to do to keep ECMA-262 sec 11.8.7 step 5 from throwing without using a try - catch.

dougwilson commented 9 years ago

According to ECMA-262 sec 15.2.4.2 the correct method is Object.prototype.toString.call(obj) === '[object Object]'.

dougwilson commented 9 years ago

Will that work for JXcore?

obastemur commented 9 years ago

No, it would show [object Function]

However, I think I've found a way to wrap native function instance to behave like an object without a performance hit. I'm calling Object.create on new native function templates (for Stat) and it does the magic.

ATM this fix is not deadly needed. Thank you very much for the assistance and help.

dougwilson commented 9 years ago

Are you absolutely positive it will show [object Function] ? Just because typeof is functions doesn't mean the toString will be function, as they have different semantics.

Can you point me how to run JXcore on Windows so I can play around with this?

obastemur commented 9 years ago

Yes. I've just double checked.

Windows SM (SpiderMonkey) build is on TODO short list. Do you have any other environment ?

dougwilson commented 9 years ago

Do you have any other environment ?

Unfortunately only Travis CI.

Looking closer, I think if I swap the genuine fs.Stats and not even an object checks, we'd be in a better shape here, at least.

obastemur commented 9 years ago

Yes. That would work perfectly.