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

Adobe ExtendScript errors #32

Merged
merged 1 commit into from
Mar 25, 2017
Merged

Adobe ExtendScript errors #32

merged 1 commit into from
Mar 25, 2017

Conversation

SheetJSDev
Copy link
Contributor

@SheetJSDev SheetJSDev commented Mar 25, 2017

Fixes #31:

  • ES parses /=+$/ as a division-assignment; replace with new RegExp
  • fallback to global eval if self and exports are not available
$ make bytes
gzip --best --stdout base64.min.js | wc -c | tr -d ' '
568
$ make test
  ․․․․․․

  6 passing (10ms)
=============================== Coverage summary ===============================
Statements   : 100% ( 22/22 )
Branches     : 85% ( 17/20 )
Functions    : 100% ( 4/4 )
Lines        : 100% ( 22/22 )
================================================================================

@davidchambers
Copy link
Owner

Thank you for submitting this pull request, @SheetJSDev!

base64.js Outdated
@@ -37,7 +37,7 @@
// [https://gist.github.com/1020396] by [https://github.com/atk]
object.atob || (
object.atob = function (input) {
var str = String(input).replace(/=+$/, '');
var str = String(input).replace(new RegExp("=+$"), ''); // #31: ExtendScript bad parse of /=
Copy link
Owner

Choose a reason for hiding this comment

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

Did you see @michaelficarra's suggestion in #31? It strikes me as a nice approach. Another option is /[=]+$/ (I just tested it in ExtendScript Toolkit).

I'm happy with any of these three fixes. @michaelficarra, do you have a preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidchambers /[=]+$/ is probably ubiquitous since any reasonable regexp library will support character sets. I don't know what engines support non-capture groups (which was @michaelficarra 's suggestion)

In the interest of parsimony /[=]+$/ only adds 2 characters to the output while /(?:)=+$/ adds 4 and new RegExp adds many more, so that is probably the best option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we come to a consensus I can change the commit, but let's wait for @michaelficarra to chime in first.

base64.js Outdated
@@ -1,6 +1,6 @@
;(function () {

var object = typeof exports != 'undefined' ? exports : self; // #8: web workers
var object = typeof exports != 'undefined' ? exports : typeof self != 'undefined' ? self : (1,eval)('this'); // #8: web workers, #31: ExtendScript
Copy link
Owner

Choose a reason for hiding this comment

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

Does this work for you? For me,

(function() { return (1, eval)('this'); }())

evaluates to a reference to the anonymous function. On the other hand,

(function(global) { return global; }(this))

evaluates to the global object.

Now that we have three cases we could reformat the ternary expressions to improve readability. I suggest something along these lines:

var object =
  typeof exports != 'undefined' ? exports :
  typeof self != 'undefined'    ? self :
  /* otherwise */                 ...;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidchambers 1,eval trick works in the illustrator context but not in the photoshop context. I couldn't get your function working either. So I went back to the docs https://wwwimages2.adobe.com/content/dam/Adobe/en/devnet/scripting/pdfs/javascript_tools_guide.pdf and it says $.global should be available everywhere.

Can you check if $.global works for you?

Also, what should happen if none of the variables are available? The original code assumed self was available

Copy link
Owner

Choose a reason for hiding this comment

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

Can you check if $.global works for you?

It does. Let's use it!

Also, what should happen if none of the variables are available?

I'm happy to leave the behaviour unspecified.

Fixes #31:
- ES parses `/=+$/` as a division-assignment
- fallback to $.global if `self` and `exports` are not available
@SheetJSDev
Copy link
Contributor Author

Amended commit with the discussed changes. Thanks @davidchambers for looking into this on a Saturday :)

Copy link
Collaborator

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM

@davidchambers davidchambers merged commit 73b3905 into davidchambers:master Mar 25, 2017
@davidchambers
Copy link
Owner

Released as 1.0.1. Thank you for the thoughtful pull request, @SheetJSDev. :)

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