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

Change this to window #4

Open
ptarjan opened this issue Jun 1, 2015 · 5 comments
Open

Change this to window #4

ptarjan opened this issue Jun 1, 2015 · 5 comments

Comments

@ptarjan
Copy link

ptarjan commented Jun 1, 2015

In https://github.com/nathanhammond/libphonenumber/blob/master/dist/libphonenumber.js#L1 the very first line is var aa = this. When using your library with browserify, your library will be in a closure whose context is not the window, but instead undefined.

Can you just change it to var aa = window? I'm sure this could be fixed in closure too, but I don't know that code well enough to open the issue.

@nathanhammond
Copy link
Owner

I feel like the root of this is an issue with Browserify. this in that closure should clearly reference the global object, and I don't want to hard-code that or make assumptions about the environment that this is going to run in. :/ Pragmatic me says "absolutely" and perfectionist me says "eh." Are there Browserify settings that can address this? (I've still never used Browserify.)

The other thing that I'm possibly game to do (some time after 6/16) is to add a UMD wrapper around everything–but I haven't any clue how to make that happen with Closure explicitly setting things on the global object.

Basically, the output has been mostly opaque to me since I built this. At this point it's not much more than a fancy wrapper around the installation instructions that exist in the libphonenumber readme–what I've done is hardly additive. (And more, reductive, which means to me that the Google libphonenumber output is at the wrong level of abstraction.)

@ptarjan
Copy link
Author

ptarjan commented Jun 2, 2015

Using https://github.com/thlorenz/browserify-shim I was able to patch your library in (it does the replacing of this with window). I tried to use https://github.com/eugeneware/debowerify to no avail, nor https://github.com/tminglei/browserify-bower. At least I have a solution if others have the same problem.

@ptarjan ptarjan closed this as completed Jun 2, 2015
@ptarjan
Copy link
Author

ptarjan commented Jun 3, 2015

I ended up with this, and it makes me sad:

gulp.task('fix-libphonenumber-non-strictness', ['install'], function() {
  var lib = './bower_components/libphonenumber/dist';
  return gulp.src(lib + '/libphonenumber.js')
    .pipe(replace('var aa=this;', 'var aa=window;'))
    .pipe(gulp.dest(lib));
});

@nathanhammond
Copy link
Owner

That's gross.

@nathanhammond nathanhammond reopened this Jun 4, 2015
@supersam654
Copy link

As a compromise between not making assumptions about the environment but still supporting browserify (and anything else that has an undefined global object), what if you did aa = this || window;?

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

No branches or pull requests

3 participants