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

Fix ramda imports (v0.25 does not support default imports) #98

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

mdimovska
Copy link
Contributor

Ramda v0.25.0 does not have a default export anymore: https://github.com/ramda/ramda/blob/master/source/index.js.
Discussion: ramda/ramda#2322

React native apps and the tests in GE were not affected with this change (I haven't investigated this in more details, might be because of a babel plugin). However, including Girders Elements in a React app with webpack causes errors (R is undefined because there is no default export in ramda).

This PR fixes the imports.

@coveralls
Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage remained the same at 83.034% when pulling 7fdd08a on fix/ramda-imports into e12b3a9 on master.

@andon
Copy link
Member

andon commented Feb 4, 2018

You did the bigger refactoring 😄. Easier would have been to just fix the import:

- import R from 'ramda'
+ import * as R from 'ramda'

Now to the more important topic, do we want to use ramda utilities without the prefix like you refactored it?

I personally used to write code like you refactored it. But, in the last year I changed my preference. We mostly use two libraries to do our FP magic, immutable and ramda. There is a cognitive load by not having them natively in JS. I often identify with the following article:
https://hackernoon.com/functional-programming-in-javascript-is-an-antipattern-58526819f21e

This is my personal preference, but I vote for staying with R. and I. when using ramda and immutable.

Copy link
Member

@andon andon left a comment

Choose a reason for hiding this comment

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

I would only fix the import and not the way we use ramda.

@mdimovska
Copy link
Contributor Author

Currently, there is a mix in the code: we are somewhere using default imports, somewhere we have multiple imports from ramda. I will make sure we have default imports everywhere (stay with R. for now).
Just a note: as discussed here: ramda/ramda#2322, they removed the default export on purpose - because of tree-shaking in webpack, so that only parts of the library are included in the bundle. But, they should have made a major release, since this is a breaking change. :)

@ognen
Copy link
Member

ognen commented Feb 5, 2018

Yes, we should also try to use named exports instead of export default for the same purposes. Also, I think it makes more sense to use R.fn in larger (more than one screen) source files. Smaller files, where you can read the imports easily, you can also import the fn directly.

@ognen ognen merged commit ab359ad into master Feb 5, 2018
@ognen ognen deleted the fix/ramda-imports branch February 5, 2018 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants