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

Add rest spread support #75

Merged
merged 2 commits into from
Jan 2, 2018
Merged

Add rest spread support #75

merged 2 commits into from
Jan 2, 2018

Conversation

bcomnes
Copy link
Member

@bcomnes bcomnes commented Dec 24, 2017

Detective was throwing when used with newer node code that uses spread operators. This adds a plugin to acorn which enables support for the spread operator until there is better built in support in acorn.

@goto-bus-stop
Copy link
Member

we can prob drop node 0.12 together with the major bump for #73

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

lgtm, do you know of any other new js features that are supported in node but not in acorn?

@bcomnes
Copy link
Member Author

bcomnes commented Dec 25, 2017

I’m in favor or dropping 0.12, given that nobody should be using it at this point. I would want to bounce that off the most active maintainer before doing so though.

I’m not totally up to speed which features we should of be supporting at this point but maybe it would be worth doing a thorough rundown at some point.

@bcomnes bcomnes requested a review from a user January 2, 2018 02:15
@bcomnes
Copy link
Member Author

bcomnes commented Jan 2, 2018

@substack interested in your opinion on what we should do wrt semver and dropping support for 0.12.

@bcomnes
Copy link
Member Author

bcomnes commented Jan 2, 2018

bret: I guess it makes sense to bump the major to drop 0.12
18:19 plenty of numbers, might as well
18:20 Bret Comnes substack: I can take care of the related 0.12 tasks related to this PR (namely, the workaround to ignore the test when run in 0.12) but I'm not to familiar with the code base. We can leave other cleanup tasks to other PRs I'm thinking
18:21 I think this will bring rest spread 'support' to browserify and fam
18:27 substack: ok I'll go ahead and do that
18:29 substack: this will cascade into a module-deps and browserify major release as well?
18:30 probably?
18:34 Bret Comnes Ok, I'll see how far I can get

@bcomnes
Copy link
Member Author

bcomnes commented Jan 2, 2018

Rolled back 0.12 support on the PR.

@bcomnes bcomnes merged commit f24f0de into browserify:master Jan 2, 2018
@bcomnes bcomnes deleted the rest-spread branch January 2, 2018 03:01
@ljharb
Copy link
Member

ljharb commented Jan 6, 2018

We absolutely should not have dropped older node support. This needs to be restored.

@bcomnes
Copy link
Member Author

bcomnes commented Jan 6, 2018

Can you elaborate? Happy to restore.

@ljharb
Copy link
Member

ljharb commented Jan 6, 2018

So far my tests in older nodes are failing in browserify 15 due solely to arrow syntax being used (in detective, i think?). That's a really silly reason to break things. Either we should stick with ES5 syntax or we should use a build process prepublish, as is npm best practice.

@bcomnes
Copy link
Member Author

bcomnes commented Jan 6, 2018

Looks like my commits that supported 0.12 were pruned in the branch. I'll push those back up for reference.

@bcomnes
Copy link
Member Author

bcomnes commented Jan 6, 2018

@ljharb I saved it actually for this very situation. Yay me from a few days ago: https://github.com/bcomnes/detective/tree/0.12

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.

None yet

3 participants