-
Notifications
You must be signed in to change notification settings - Fork 243
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
Make rbush an ES6 library #89
Conversation
PS Just looking at Travis and it looks like it's unhappy because of the use of ESM which relies on Node v6 where as travis is configured to run against v4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking on this! Would you mind doing some changes to make it more in line with my conventions for other libraries (in terms of entry points, rollup config, etc.)? E.g. you could use Flatbush as a model — https://github.com/mourner/flatbush/blob/master/rollup.config.js
No worries, stay tuned |
Hi @mourner It's now pretty much inline with how Cheers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now! Let's also update .travis.yml
to use Node v8 instead of v4 to make sure the build is green.
.gitignore
Outdated
rbush.js | ||
rbush.min.js | ||
rbush.es.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Hi @mourner Sorry to bump this but any chance you could get this merged and published so I can do some work on Turf over the weekend coming up? Thanks, |
Would love for this PR to be merged since I get problems with some custom packaging of ES6 which relays on all packages to be ES6 modules. |
Similar to @eljenso, I'd really like this to be merged before I forget I'm using the fork! |
Hi @mourner
I didn't get a response on #88 but figured I'd proceed with throwing together a pull request anyway given that it wasn't a huge amount of work, so hopefully you're happy to accept it :)
The reason I'm pretty keen for this is to support TurfJS as we're trying to make everything ES6 and tree-shaking is problematic unless our dependencies are ES6 as well.
Most things should be fairly similar to how you'd set them up for
quickselect
, probably the primary thing I changed was moving frominstanbul
tonyc
for the coverage tests - I can't confess to know a huge amount about that coverage ecosystem but hopefully what I've done makes sense.Let me know if you have any questions or concerns.
Thanks,
Rowan