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

build based on node 5.7.0 #186

Merged
merged 2 commits into from
Mar 11, 2016
Merged

build based on node 5.7.0 #186

merged 2 commits into from
Mar 11, 2016

Conversation

calvinmetcalf
Copy link
Contributor

this will probably end up in a readable-stream release

@calvinmetcalf
Copy link
Contributor Author

all the one off fixes for arrow functions are pretty untenable, so this version uses babel, which adds 2 wrinkles

  • some of the replacements need to be rewritten to use const or let because now those are being fixed after the replacements
  • a couple tests have top level returns so need to be wrapped in an iefe (which is why babel must go after the replacements).

babel does something funky with the white space so while I have retainLines on right now to be able to eye ball the changes we probably want to turn that off.

new build uses babel for arrow functions and const/let stuff
also makes iThings allowed failures
@calvinmetcalf
Copy link
Contributor Author

@mcollina
Copy link
Member

what's the output without retainLine?

I would prefer different line numbers from core but a more readable thing. Line numbers can potentially be different anyway (because of the replacements).

@calvinmetcalf
Copy link
Contributor Author

It puts conditionals that don't have curly braces are put on to a single
line, not a big deal but it makes comparing the diff tricky, I can rerun it
with that off now that it passes.

On Fri, Feb 26, 2016, 3:07 AM Matteo Collina notifications@github.com
wrote:

what's the output without retainLine?

I would prefer different line numbers from core but a more readable thing.
Line numbers can potentially be different anyway (because of the
replacements).


Reply to this email directly or view it on GitHub
#186 (comment)
.

@calvinmetcalf
Copy link
Contributor Author

ok removed retain lines

@mcollina
Copy link
Member

👍. I would bump the minor here, just because it's a significant change (babel), and my change might or might not land in v4.x.x.

@calvinmetcalf
Copy link
Contributor Author

hm I tend to not count build changes in terms of features, but it is a
largish change so I'm on the fence here

On Fri, Feb 26, 2016 at 8:52 AM Matteo Collina notifications@github.com
wrote:

[image: 👍]. I would bump the minor here, just because it's a
significant change (babel), and my change might or might not land in v4.x.x.


Reply to this email directly or view it on GitHub
#186 (comment)
.

break;
else
len = state.length;
break;else len = state.length;
Copy link
Member

Choose a reason for hiding this comment

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

this looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, but it is an artifact of babel transforming the code

@calvinmetcalf
Copy link
Contributor Author

hey guys can I get a yay or nay on this one

@mcollina
Copy link
Member

LGTM

@calvinmetcalf calvinmetcalf merged commit c9364ba into master Mar 11, 2016
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