Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Es6module#60

Merged
esjewett merged 13 commits into
crossfilter:mastercrossfilter/reductio:masterfrom
preignition:ES6modulepreignition/reductio:ES6moduleCopy head branch name to clipboard
Feb 2, 2020
Merged

Es6module#60
esjewett merged 13 commits into
crossfilter:mastercrossfilter/reductio:masterfrom
preignition:ES6modulepreignition/reductio:ES6moduleCopy head branch name to clipboard

Conversation

@christophe-g

@christophe-g christophe-g commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

Port to ES6 module, same exercice as crossfilter/crossfilter#142

Tests are passing against the build file (reductio.min.js). Struggling to make them pass against src files due to karma/jasmine having trouble properly interpreting import statement.

Travis build failing, potentially due to old node engine (0.10). Running fine locally on my machine (node 10.16.3).

@christophe-g

Copy link
Copy Markdown
Contributor Author

Dear @esjewett

Now that crossfilter runs Es6 modules, any chance to get this reviewed ?
Thanks,
C.

@esjewett

Copy link
Copy Markdown
Member

@christophe-g Yup, I can look at this. Is this ready to review?

@christophe-g

Copy link
Copy Markdown
Contributor Author

@esjewett - excellent thanks.

Yes it is from my perspective. I am using this branch without apparent problems.

As said above, test seems to need more recent nodejs than the one in travis.

@esjewett

Copy link
Copy Markdown
Member

@christophe-g Ok, looking good, and confirmed that the tests are passing.

Two questions:

  • Are you able to update the Travis node version in the .travis.yml file in your branch, or does that not work?
  • What would we expect this to break? I'm guessing this will be a major version bump as it will break pretty much all existing usage of the library?

@christophe-g

christophe-g commented Jan 28, 2020

Copy link
Copy Markdown
Contributor Author

That was quick !

Are you able to update the Travis node version in the .travis.yml file in your branch, or does that not work?

Done, travis success.

What would we expect this to break? I'm guessing this will be a major version bump as it will break pretty much all existing usage of the library?

It will break ie. Yes it should be a major version bump. I suggest we get inspiration from latest crossfilter release (https://github.com/crossfilter/crossfilter/releases/tag/1.5.0) for release note.

@esjewett

Copy link
Copy Markdown
Member

Excellent - thank you. I will try to get this merged and released today or this weekend.

@esjewett
esjewett merged commit ff091fe into crossfilter:master Feb 2, 2020
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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.