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

alwaysEmptyObject and alwaysEmptyArray do not always return empty array or object #97

Closed
nfantone opened this issue Jun 19, 2018 · 7 comments

Comments

@nfantone
Copy link
Contributor

Output from alwaysEmptyArray and alwaysEmptyObject cannot be trusted to be always empty. Specially if their outputs are being passed around as inputs to third party libraries. This can lead to hard to track bugs and odd behaviors.

R_.alwaysEmptyArray(); // []
R_.alwaysEmptyArray().push(42); 
R_.alwaysEmptyArray(); // [42] <- return value has been changed forever for every other subsequent call

I propose we either:

  • Change the name of the functions to reflect the above fact.
  • Return a R.clone of the emptyArray or emptyObject each time:
const alwaysEmptyArray = () => clone(emptyArray);
  • Leverage the new (unreleased as of yet) R.thunkify function to create a point-free reusable factory of clones and implement other always* functions based off of it.
const alwaysNew = thunkify(clone);
const alwaysEmptyArray = alwaysNew(emptyArray);

DISCLAIMER: I was the author of the thunkify PR.

@tommmyy
Copy link
Owner

tommmyy commented Jun 20, 2018

Thank you for the catch! I like the thunkify version :)
Do you want to send us PR, @nfantone? ;)

@tommmyy
Copy link
Owner

tommmyy commented Jun 20, 2018

...although we must wait for Ramda release which will contain thunkify.

@nfantone
Copy link
Contributor Author

@tommmyy Gladly do so when they decide to cut a release! Although we could write thunkify ourselves and just replace it when the time comes. It's fairly simple.

Your call.

@tommmyy
Copy link
Owner

tommmyy commented Jun 20, 2018

@nfantone ok, agreed. You can temporarily add thunkfify function. Thank you!

@nfantone
Copy link
Contributor Author

nfantone commented Jun 23, 2018

@tommmyy I was trying to get a PR together for this today and found out that I cannot install the project on a fresh clone (running on macOS, node v10.4.1 and npm 6.1.0).

The installation for fsevents seems to fail. It tries to download binaries, errors and defaults to compile from source which also fails.

❯ git clone [email protected]:nfantone/ramda-extension.git
❯ cd ramda-extension
❯ npm i

> [email protected] install /Users/nfantone/Development/js/ramda-extension/node_modules/fsevents
> node install

node-pre-gyp ERR! Tried to download(404): https://fsevents-binaries.s3-us-west-2.amazonaws.com/v1.1.3/fse-v1.1.3-node-v64-darwin-x64.tar.gz
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (node-v64 ABI, unknown) (falling back to source compile with node-gyp)
node-pre-gyp ERR! Tried to download(undefined): https://fsevents-binaries.s3-us-west-2.amazonaws.com/v1.1.3/fse-v1.1.3-node-v64-darwin-x64.tar.gz
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (node-v64 ABI, unknown) (falling back to source compile with node-gyp)

[...bunch more...]

make: *** [Release/obj.target/fse/fsevents.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at ChildProcess.emit (events.js:182:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12
gyp ERR! System Darwin 17.6.0

[...even more...]

Removing package-lock.json solved the issue for me. Incidentally, I found out that it does work with yarn out of the box. I'm guessing one of the lockfiles is not up-to-date?

@nfantone
Copy link
Contributor Author

@tommmyy Just found out that you guys are using workspaces.

  "workspaces": [
    "packages/*"
  ],

So, ramda-extension only supports yarn. Am I correct?

@tommmyy
Copy link
Owner

tommmyy commented Jun 24, 2018

@nfantone yes, you are correct! It is our bad, that we kept package-lock.json. I am gonna to remove it. I added issue for that.

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

No branches or pull requests

2 participants