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.

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

Date.prototype.toISOString called on non-finite value with Adobe Scripting #365

Closed
iby opened this issue Dec 10, 2015 · 31 comments
Closed

Comments

@iby
Copy link

iby commented Dec 10, 2015

Trying to make use of es5-shim with Adobe Scripting, but getting the following exception:

image

Trying to do a very basic thing with:

require('es5-shim');

$.writeln(Object.keys({
    "foo": 1,
    "bar": 2
}));
@ljharb
Copy link
Member

ljharb commented Dec 10, 2015

Do you know if it's the require that causes it, the $.writeln call, or the Object.keys call?

@iby
Copy link
Author

iby commented Dec 10, 2015

It's Object.keys. Everything else works fine on it's own. I'm able to do Object.keys with MDN's polyfill, but adding polyfills for everything is reinventing the wheel.

image

@iby
Copy link
Author

iby commented Dec 10, 2015

Well… actually, sorry. The error gets thrown from the require part, if I understand correctly it never gets to Object.keys.

image

It must be some other shim that causes this:

image

@ljharb
Copy link
Member

ljharb commented Dec 10, 2015

Can you confirm whether or not try/catch works in Adobe scripting? the es5-shim throws errors frequently to detect runtime bugs, but it wraps them all in a try/catch. I'm wondering if Adobe ignores that.

@iby
Copy link
Author

iby commented Dec 10, 2015

Yep.

try {
    require('es5-shim');
} catch (error) {
    $.writeln('Caught an error:', error);
}

// Caught an error:Error: Date.prototype.toISOString called on non-finite value.

$.writeln(Object.keys({
    "foo": 1,
    "bar": 2
}));

// foo,bar

Object.keys also works!

@ljharb
Copy link
Member

ljharb commented Dec 10, 2015

OK, that's great! It means it's fixable. Does error.stack contain anything? That would be most helpful (ie, not just the line that throws, but the stack trace that led there)

@iby
Copy link
Author

iby commented Dec 10, 2015

stack is undefined. those are the properties of the error: number, fileName, line, source, start, end, message, name, this.

@ljharb
Copy link
Member

ljharb commented Dec 10, 2015

Sorry for the back and forth - if any of that info points to anything besides that toISOString error, that would be very helpful.

@iby
Copy link
Author

iby commented Dec 10, 2015

Happy to help, don't worry. Nope, my understanding is that it goes from top to bottom – it hits the first issue and throws an error, meaning there might be something further down the line. I just tried disabling the defineProperties execution, no other errors were thrown, fingers crossed this is the only one.

@ljharb
Copy link
Member

ljharb commented Dec 10, 2015

ok a few more questions - what does new Date(-1).getTime() and new Date(-62198755200000).getTime() return?

@iby
Copy link
Author

iby commented Dec 10, 2015

$.writeln(new Date(-1).getTime());
// -1

$.writeln(new Date(-62198755200000).getTime());
// -62198755200000

@iby
Copy link
Author

iby commented Dec 12, 2015

@ljharb I noticed you pushed a bunch of commits recently. Any chance they resolve this case? I'd offer a pr, but assume that this is something tricky?

@ljharb
Copy link
Member

ljharb commented Dec 12, 2015

No, I haven't resolved this yet. Since I can't repro it myself, and since I don't know exactly what's failing, I'm a bit lost.

Can you confirm that if you comment out the hasNegativeDateBug and hasSafari51DateBug checks (replacing them with a literal false, for example), that the shim loads successfully? Even if I don't know why the lines are failing, knowing which ones are throwing would help.

@iby
Copy link
Author

iby commented Dec 13, 2015

Yep, setting those two to false eliminates the error, that's what I meant in #365 (comment). isFinite is causing a problem with Date that was constructed with a string with a negative number. See this:

$.writeln(isFinite(new Date(-1)));
// true
$.writeln(isFinite(new Date('-1')));
// false
$.writeln(isFinite(new Date(-62198755200000)));
// true
$.writeln(isFinite(new Date('-62198755200000')));
// false
$.writeln(isFinite(new Date(1)));
// true
$.writeln(isFinite(new Date('1')));
// true

…some time later: Oh dear, I tried finding workaround and discovered that the first time shim is applied – it works, but goes wrong when it's being run the second time… That is because Adobe keeps the JS environment for the entire app session, so here's what happens:

  1. Script checks those two things, doesn't find toISOString and successfully adds it.

    var hasNegativeDateBug = Date.prototype.toISOString && new Date(negativeDate).toISOString().indexOf(negativeYearString) === -1;
    var hasSafari51DateBug = Date.prototype.toISOString && new Date(-1).toISOString() !== '1969-12-31T23:59:59.999Z';
  2. The second time it runs it finds toISOString and executes it. Apparently there is also no getUTC… methods, because they are failing next, though they are specified in the older docs. But the latest version differs from CS6 – nothing surprising it's gone.

  3. So, because I don't use toISOString it doesn't fail, but it uses it itself the second time it runs.

Updating the following block works and shouldn't affect other environments.


var hasUTCMethods = Date.prototype.getUTCFullYear && Date.prototype.getUTCMonth && Date.prototype.getUTCDate && Date.prototype.getUTCHours && Date.prototype.getUTCMinutes && Date.prototype.getUTCSeconds && Date.prototype.getUTCMilliseconds;
var hasNegativeDateBug = Date.prototype.toISOString && new Date(negativeDate).toISOString().indexOf(negativeYearString) === -1;
var hasSafari51DateBug = Date.prototype.toISOString && new Date(-1).toISOString() !== '1969-12-31T23:59:59.999Z';

hasUTCMethods && defineProperties(Date.prototype, {

@ljharb
Copy link
Member

ljharb commented Dec 26, 2015

OK, so - you're saying that CS doesn't have any of Date#{getUTCFullYear, getUTCMonth, getUTCDate, getUTCHours, getUTCMinutes, getUTCSeconds, getUTCMilliseconds}?

If not, I'll want to shim all of those as well.

ljharb added a commit that referenced this issue Dec 27, 2015
…doesn’t observably look them up on the receiver.

 - Date#getUTCFullYear
 - Date#getUTCMonth
 - Date#getUTCDate
 - Date#getUTCHours
 - Date#getUTCMinutes
 - Date#getUTCSeconds
 - Date#getUTCMilliseconds

This should also help expose the true cause of #365.
@ljharb
Copy link
Member

ljharb commented Dec 27, 2015

@ianbytchek if you could try checking on latest master, I suspect we'll now see an error on the first sourcing of the file, because those UTC methods aren't a function.

@iby
Copy link
Author

iby commented Dec 27, 2015

OK, so - you're saying that CS doesn't have any of Date#{getUTCFullYear, getUTCMonth, getUTCDate, getUTCHours, getUTCMinutes, getUTCSeconds, getUTCMilliseconds}?

That's right @ljharb. Just checked the master, can't see any difference in behaviour, it doesn't fail the first time and fails on consecutive runs like before on this line:

throw new RangeError('Date.prototype.toISOString called on non-finite value.');

Might have you missed the point about the constant js environment? The main problem is that shim sees different things when it checks at what needs to be done when it runs for the first time and when it runs consecutively in the same environment.

@ljharb
Copy link
Member

ljharb commented Dec 27, 2015

@ianbytchek yes, I got that :-) the issue seems to be that the toISOString shim depends on the presence of those UTC methods. however, the RangeError suggests it might be something else. Could you try editing that line - 1278 in my copy - to somehow print out or console.log the this value? I'm curious what it is that's not finite.

@iby
Copy link
Author

iby commented Dec 28, 2015

@ljharb just making sure 😊 See below.

$.writeln(this);
// Fri Jan 01 -1 04:00:00 GMT+0400
$.writeln(typeof this);
// object

@ljharb
Copy link
Member

ljharb commented Dec 28, 2015

@ianbytchek Thanks - what about +this and Number(this) and this.getTime()?

@ljharb
Copy link
Member

ljharb commented Feb 9, 2016

@ianbytchek any update? sorry for all the back and forth, but without a copy of Photoshop for my to test myself, it's hard for me to verify that a fix will work.

@iby
Copy link
Author

iby commented Feb 10, 2016

@ljharb sorry, this got out of hands. I'll look into this towards the end of the week and will report. Happy to help.

@ljharb
Copy link
Member

ljharb commented Feb 10, 2016

Thanks, much obliged!

@iby
Copy link
Author

iby commented Feb 14, 2016

@ljharb here's what I've got. What do you think?

if (!isFinite(this)) {
    $.writeln(this); // Fri Jan 01 -1 04:00:00 GMT+0400
    $.writeln(typeof this); // object
    $.writeln(+this); // NaN
    $.writeln(Number(this)); // NaN
    $.writeln(this.getTime()); // -62198755200000

    throw new RangeError('Date.prototype.toISOString called on non-finite value.');
}

@ljharb
Copy link
Member

ljharb commented Feb 14, 2016

@ianbytchek Thanks! From here I think I can figure out what might be the issue - if you want to try making the condition if (!isFinite(this) && !isFinite(this.getTime()) { and see if that fixes your problem, that's my hunch.

It's super bizarre that a Date object would have a finite getTime() but coerce to a number that's different than that. Presumably var date = new Date(-62198755200000); [+date, Number(date), date.getTime()] also produces [NaN, NaN, -62198755200000]?

@iby
Copy link
Author

iby commented Feb 20, 2016

@ljharb replacing condition with if (!isFinite(this) && !isFinite(this.getTime()) { worked! Also, yes, doing that produces the expected outcome. Please update master.

if (!isFinite(this)) {
    var date = new Date(-62198755200000);
    $.writeln(+date); // NaN
    $.writeln(Number(date)); // NaN
    $.writeln(date.getTime()); // -62198755200000

    throw new RangeError('Date.prototype.toISOString called on non-finite value.');
}

@ljharb ljharb closed this as completed in 6352df1 Feb 21, 2016
@ljharb
Copy link
Member

ljharb commented Feb 21, 2016

Released as v4.5.5

@iby
Copy link
Author

iby commented Feb 22, 2016

👍 🍻

@ljharb
Copy link
Member

ljharb commented Feb 22, 2016

Thanks for reporting this and following through back and forth :-) I love these kinds of quirky environment issues.

@iby
Copy link
Author

iby commented Feb 22, 2016

Try do something with adobe scripting, bet you'll change your mind! 😄 But there is that strange satisfaction… like when taking an old car, stuffing it with latest technology and see it rolling.

@matsilva
Copy link

Thanks guys, this just helped me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants