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

"for of" loop is not supported by legacy browsers. #248

Merged
merged 5 commits into from
Jan 30, 2018

Conversation

rcastera
Copy link
Contributor

@rcastera rcastera commented Jan 26, 2018

-Adding babel-polyfill for supporting older browsers
-Resolves #247
-Resolves error ReferenceError: Can't find variable: Symbol in bower_components/twitter-text/js/pkg/twitter-text-2.0.2.js (line 854)

@kaushlakers
Copy link
Contributor

I don't think we need to whole polyfill package for just startsWIth. I've substituted it with substring in this patch.

#244

@rcastera
Copy link
Contributor Author

rcastera commented Jan 26, 2018

@kaushlakers This isn't just for startsWith. I still have an issue with twitter-text running in Phantomjs failing with this error ReferenceError: Can't find variable: Symbol in bower_components/twitter-text/js/pkg/twitter-text-2.0.2.js (line 854).

We have many tests that are failing when upgrading to the newer version of twitter-text due to lack of support. One of your dependencies bundled in the library has the same issue: punycode

This error exists in the travis builds as well: https://travis-ci.org/twitter/twitter-text/jobs/333552627

@rcastera
Copy link
Contributor Author

See this for an explanation: babel/babel-loader#84

@kaushlakers
Copy link
Contributor

@rcastera good point. I don't think it's twitter-text's responsibility as a text parsing library to polyfill browser features for legacy browsers. We use this polyfill internally.

As a follow up, I can try to raise a PR to the punycode.js lib to replace the "for of" loops. From what I can see, we can safely change it to use a for loop since it is always an array.

@kaushlakers
Copy link
Contributor

@hansott are you planning to raise a PR to punycode or shall I go ahead? :)

@hansott
Copy link

hansott commented Jan 29, 2018

@kaushlakers Feel free to do so 😉

@rcastera
Copy link
Contributor Author

Thanks @kaushlakers @hansott

@kaushlakers will you create a PR to apply this patch as well?

@kaushlakers
Copy link
Contributor

kaushlakers commented Jan 29, 2018

Created this PR. Hoping we can get this merged and published. We can apply @hansott's patch to twitter-text once that is resolved or @rcastera feel free to update this PR to apply that patch. 😄

@kaushlakers kaushlakers reopened this Jan 29, 2018
@kaushlakers kaushlakers changed the title adding babel-polyfill "for of" loop is not supported by legacy browsers. Jan 29, 2018
@rcastera
Copy link
Contributor Author

@kaushlakers I'll update this PR with the patch. Thanks!

@rcastera
Copy link
Contributor Author

@kaushlakers updated the PR. I also removed this unused variable

Copy link
Contributor

@kaushlakers kaushlakers left a comment

Choose a reason for hiding this comment

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

Looks like the rake package command was slightly wrong as it missed a build step. I've updated the README with the correct command. Can you rerun and generate the standalone pkg files again (with the rc tag i.e. 2.0.4-rc1)? Looks like this is missing punycode bundled in.
https://github.com/twitter/twitter-text/blob/master/js/README.md#packaging

kaushlakers and others added 3 commits January 29, 2018 13:18
Added an rc tag to the version. I'd prefer to release a full version after we resolve the issue in punycode as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants