Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@elingerojo
Copy link
Contributor

Fixes #65

Approach

  1. One step, one commit. For easy review.
  2. Start by setting the wanted Node support to work toward getting the Travis CI green arrow.
  3. Only remove syntactic sugar to be ES5 compatible
  4. Squash commits at merge if desired

@zeke
Copy link
Contributor

zeke commented May 1, 2017

It looks like class OnigRegExp was already commented out. What about just removing that code instead of converting it to ES5?

@elingerojo
Copy link
Contributor Author

I did the comment out after converting to ES5 and left the comments for easy review.
The change is needed to get back Node 4 support

  • Updated with a new commit with comments removed just now

@zeke
Copy link
Contributor

zeke commented May 2, 2017

Looks good to me. What do you think, @maxbrunsfeld?

@ashleygwilliams
Copy link

would love to know the status on this!

@zeke
Copy link
Contributor

zeke commented Sep 5, 2017

Let's make it happen!

Do we want to first update the travis config to test on more node versions? 4, 6, 8?

@ashleygwilliams
Copy link

thanks @zeke! 4, 6, and 8 seems good to me. i'm happy to make the travis PR if you'd like as well. just lemme know!

@ashleygwilliams
Copy link

@zeke actually i just opened #70 ;) waiting on travis now

@ashleygwilliams
Copy link

@zeke ? would love to get this merged

@zeke zeke mentioned this pull request Sep 8, 2017
@zeke
Copy link
Contributor

zeke commented Sep 8, 2017

I want to see this passing on node 4, 6, and 8 first. I can't edit this PR, so I've opened a followup to it: #71

@zeke zeke closed this Sep 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants