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

document polyfill for phantomjs compatibility #838

Closed
wants to merge 1 commit into from
Closed

document polyfill for phantomjs compatibility #838

wants to merge 1 commit into from

Conversation

alexdean
Copy link

see discussion in #578.

if there's a better place for this documentation, i'm happy to move it.

cc: @yuvii

@aendra-rininsland
Copy link
Member

Some thoughts:

  • On one hand, it's an issue solely with PhantomJS, which is annoying because it means either we should either signpost the polyfill as per the above, or we add the (albeit rather short) polyfill to the codebase itself. Given the unminified version of the polyfill is 158 bytes, it's really either a trade-off between ever-so-slight code bloat or cognitive dissonance for PhantomJS users who don't read the README (which ultimately ends up in the issue queue or forum, slowing adoption for new users and providing more work for the maintainers). @masayuki0812 — this is kind of a "project philosophy" question, which do you think is better? Personally, my thought is to just include the polyfill in the next release.
  • Failing that, this looks good, though I'm not sure whether that should be so near the beginning. We already have a problem with new users using the issue queue for support queries; I fear pushing down the notice about the issue queue and Google Group will decrease the visibility of that message.

@masayuki0812
Copy link
Member

Thank you for the PR, but I agree with @Aendrew . The polyfill is very small and this issue looks popular, so it seems reasonable from the point of view of trade-off.
Then, I'll add the polyfill in the next release v0.4.9.

@masayuki0812
Copy link
Member

I added the polyfill as default and now we don't mention it in README. So please let me close this issue. Thanks.

@alexdean
Copy link
Author

@masayuki0812 Makes sense to me. Thanks!

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