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

[BUG] ES6 default export in 0.25v #2322

Closed
allangomes opened this issue Oct 9, 2017 · 34 comments
Closed

[BUG] ES6 default export in 0.25v #2322

allangomes opened this issue Oct 9, 2017 · 34 comments
Labels

Comments

@allangomes
Copy link

import R from 'ramda'

R is not defined. :(

@allangomes allangomes changed the title [BUG] ES6 default export [BUG] ES6 default export in 0.25v Oct 9, 2017
@ecwyne
Copy link
Contributor

ecwyne commented Oct 9, 2017

it looks like source/index.js has all of the named exports but not a default export.

import R, {prop} from 'ramda';

prop('name', {name: 'ramda'}); // works
R.prop('name', {name: 'ramda'}); // breaks

Looks like export * from may provide a solution

Currently this will work

import * as R from 'ramda';

@ecwyne
Copy link
Contributor

ecwyne commented Oct 9, 2017

cc @Andarist @kedashoe

Looks like you were primary on #2254

@Andarist
Copy link
Contributor

Andarist commented Oct 9, 2017

Well, I've forgotten a little bit about this effect of the changes, was focused more on providing backward compatibility for commonjs targets.

Nevertheless I believe ramda shouldn't support import R from 'ramda' syntax. It would effectively prevent tree-shaking wins it got with my PR. Excellent explanation why it should not be supported can be read here.

As mentioned - import * as R from 'ramda'; is the way to go. If some prefer the former way of importing the "default" R - this could be easily added in the existing ramda babel plugin.

I guess this whole thing need to be explained in the migration guide, cc @kedashoe

@cloud-walker
Copy link

I personally prefer to use import R from 'ramda' syntax during development to have quickly access to all the ramda functions without import them one by one.

@davidchambers
Copy link
Member

Could you use import * as R from 'ramda'; instead, @cloud-walker?

@Andarist
Copy link
Contributor

yeah, import * as R from 'ramda' - u should be able to access all the ramda functions that way - it's basically saying "import everything from the ramda under R namespace".

@ecwyne
Copy link
Contributor

ecwyne commented Oct 10, 2017

This command helped me find-and-replace all instances of import R from 'ramda' and make them import * as R from 'ramda' (on my mac)

find . -type f -name '*.js' -exec sed -i '' s/"import R from 'ramda'"/"import * as R from 'ramda'"/ {} +

@cloud-walker
Copy link

Yes I can do (if working on existing module):

import * as R from 'ramda'
import {prop, pipe, map, chain} from 'ramda'

Feels weird but it works ❤️

So it's an intentional change?

@buzzdecafe
Copy link
Member

buzzdecafe commented Oct 10, 2017

given the comment explaining the benefit of not having a default export of the entire package, I am with @Andarist that it is the correct thing to do.

@CrossEye
Copy link
Member

Yes, I think this is just a documentation issue.

@hoschi
Copy link

hoschi commented Oct 11, 2017

the search/replace snippet from @ecwyne didn't work for me, I used:
sed -i -- "s/import R from 'ramda'/import * as R from 'ramda'/g" src/**/*.js

@buzzdecafe
Copy link
Member

closing, as this is desired behavior.

laem added a commit to betagouv/mon-entreprise that referenced this issue Oct 13, 2017
Redux-form 7 provoque une boucle infinie dans les formulaires
Ramda 0.25 ne peut plus être importé en défaut :
ramda/ramda#2322

 Please enter the commit message for your changes. Lines starting
@SeanCannon
Copy link

why is there so much resistance to adding a default export? Ramda will literally be my only import now that requires an asterisk. It also means I need to refactor like 400 files in 20 projects. Seriously this is weak.

@SeanCannon
Copy link

That thumbs down isn't going to coerce me into using Sanctuary, @davidchambers . I think it's a legitimate argument that when a library "minor" "0.___.x" upgrade requires that I touch literally every file in my app it's short-sighted and damaging.

@Andarist
Copy link
Contributor

There are technical arguments why this is better. When you also think about it ramda, as utility library, has NO default export, it is merely a namespace for bunch of utility functions.

0.x is treated differently (at least by node's semver ranges) and it doesn't give you guarantees that a minor bump won't have breaking changes.

Also I don't see why you find needed refactor to be problematic - it literally can be done with simplest find & replace.

@davidchambers
Copy link
Member

I don't consider this comment constructive, @SeanCannon:

Seriously this is weak.

You're welcome to make your case for changing something about Ramda. If the project's maintainers aren't persuaded by your argument you're of course welcome to fork the project and make changes as you see fit.

@davidchambers
Copy link
Member

The benefits of having R. in front of the functions in my projects are ten fold.

import * as R from 'ramda'; means all Ramda functions are members of R, doesn't it? If not, which module system and accompanying tooling are you using?

@SeanCannon
Copy link

It does, and in my opinion, which is founded on having used this syntax for over a year now, import R from 'ramda' should do the exact same thing.

Dude if this is a decision you guys want to move toward, you gotta give us warning. In my opinion, this is an oversight in the ES6 conversion and it's an extremely easy thing to accommodate on your end. The refusal to do it out of whatever principle you're leaning on is just not cool. You can't expect people to make a change to every file in their app on a whim. Even if it's from a script - regression testing, QA, etc.. it's all time and money.. for what? I haven't actually heard any legitimate reason from the Ramda team other than "this is desired behavior". It's not.

@SeanCannon
Copy link

I'll take responsibility for using the caret in the semver and trusting the Ramda team wasn't going to do anything drastic without a deprecation notice. I'm upset because you guys broke that trust - it wasn't just a mistake that was quickly corrected or that the team owned up to and is showing some initiative to fix - this is an outright refusal. "Your apps all broke - this is desired behavior". Can you understand my perspective now?

@Andarist
Copy link
Contributor

You do realize that you do not have to upgrade your ramda to the newest version if you do not like the changes, right? Open source is done for free by passionate people and most people just benefit from it, your rant is really out of place.

Have you also noticed that this change was done some time ago already and you are literally the only one with such hateful and dissatisfied response?

@Andarist
Copy link
Contributor

Andarist commented Nov 28, 2017

Also - changing your imports programatically is not that hard, you can do it with a codemod script. If you want you can use a simple babel transform to achieve your goal such as this:

export default function (babel) {
  const { types: t } = babel;
  
  return {
    visitor: {
      ImportDeclaration(path, state) {
        if (path.get('source.value').node !== 'ramda') {
          return          
        }

        if (t.isImportNamespaceSpecifier(path.get('specifiers.0').node)) {
          return          
        }

        path.replaceWith(
          t.importDeclaration(
            [t.importNamespaceSpecifier(t.identifier('R'))],
            t.stringLiteral('ramda')
          )
        );
      }
    }
  };
}

@SeanCannon
Copy link

There's no hate - there's extreme disappointment. I loved Ramda and everything it was doing for JS. This decision feels very not open source. It feels very proprietary and top-down "suck it up".

I'm not against the technical decision. I'm against how it was handled. It was just "noticed" and then "closed".

@SeanCannon
Copy link

image

@Andarist
Copy link
Contributor

Andarist commented Nov 28, 2017

Previous versions of ramda are very much useable, introducing a backward compatibility for this would literally prevent the main benefit of the change - better tree-shakeability for no reason other than longer migrations and you being able to bump your ramda's version, which I don't see why you would have to do - other than "having the newest".

https://docs.npmjs.com/misc/semver#caret-ranges-123-025-004
It describes that breaking changes may happen between minor releases for 0.x versions AND that caret actually won't let npm install a higher minor in that case.

@SeanCannon
Copy link

Why not make this a 1.0.0 release then? #curious

@Andarist
Copy link
Contributor

#2337 (comment)

Note - Im not the core team member

@SeanCannon
Copy link

To close - I'm not upset at the decision so much that I'm upset that you guys somehow can't possibly fathom how your decision could negatively affect people who depend on Ramda to work. Ramda is this awesome lib that just "works". My tests just "work". My compositions just "work". All of a sudden, I can't rely on Ramda to just work anymore and it has to go in the bucket of all the other unstable libraries I need to worry about. You thinking that I'm the only one affected by this proves my point. Apologies if my tone upset you.

@Andarist
Copy link
Contributor

I haven't said that you are the only one affected - I've stated only the fact that so far only you have raised so harsh statements about this change. I also still strongly believe that updating your deps to the newest version is not obligatory.

@SeanCannon
Copy link

for no reason other than longer migrations and you being able to bump your ramda's version

@CrossEye
Copy link
Member

I got sick again and dropped the ball on the 1.0 discussion. I will be getting back to that next week.

When we hit 1.0, we expect to be following the norms of semver.

@AdamMcCormick
Copy link

AdamMcCormick commented Jan 5, 2018

I'm just going to chime in with @SeanCannon here since you seem to think he's alone in this. This is a big breaking change and really shouldn't have been snuck in through a minor version. "We're not 1.0 yet" is an insufficient excuse. This is a very well used library with lots of users and including a default export helps many of them while not hurting the project in any meaningful way. It may make "sense" to you but it does more harm than it does good.

@cooperka
Copy link

cooperka commented Mar 20, 2018

I love what you're doing with this library overall, but I agree with the sentiment that major changes like this need to be better communicated to your users.

I'm glad this issue finally showed up in search results; I spent several hours debugging why in the world R suddenly became undefined and was on the verge of completely uninstalling it. I was using v0.25.0 successfully with a default import for several weeks (honestly I still don't know how this worked, but it did) and after installing Ramda on an external dependency, all libs which depended on that external library broke.

Migrating from the standard import R from 'ramda' to the non-standard import * as R from 'ramda' is still not listed on the 0.25.0 upgrade guide; it's buried in the comments.

I sincerely appreciate all your hard work, but I hope you'll consider being more proactive next time -- small, avoidable oversights like this cause loyal users to lose trust.

@CrossEye
Copy link
Member

Hmm, I don't know if someone added it since you posted, but that's listed as the first change in the upgrade guide.

You're right that we fell down on this one. I'm working on trying to get 1.0 out the door, and expect that we'll have a better process for this afterward.

@cooperka
Copy link

Yes, it looks like @kedashoe updated the guide a couple hours ago. Much appreciated 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests