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

Workaround for outdated NodObjC dependency #451

Closed
wants to merge 3 commits into from
Closed

Workaround for outdated NodObjC dependency #451

wants to merge 3 commits into from

Conversation

kbtz
Copy link

@kbtz kbtz commented Feb 21, 2018

This PR is just to illustrate what is preventing node 9 support (#426).
It seems that the latest version for the package nodobjc published on npm is more than 2 years old. Same goes for some of its dependencies that are still using native methods flagged as deprecated for a while now but finally got removed from node / v8 engine.

Thankfully the package sources on github are fresh enough so I could build all depencies using node 9 and cerebro seems to be working just fine. As shown in this PR the only tweaks required was to fetch nodobjc from github as well to make nodobjc fetch ffi from github too (hence my fork on nodobjc).

Maybe @TooTallNate have good reasons to not publish from the latest source (maybe lots of people without packege locks?), but since this project requires node 8+ this update isn't a breaking change so we could simply fetch these dependencies directly from github in case these npm packages aren't going to get updated anytime soon.

Either way I just wanted to point what I think is the source of this issue.

@maximbaz let me if that fixes this on your env too.

@maximbaz
Copy link
Contributor

This is good, but you also need to update app/yarn.lock file, because when I run cd app; yarn (according to README) it will read yarn.lock and not package.json.

@maximbaz
Copy link
Contributor

I'm not sure if you did this intentionally, but you removed yarn.lock and created a different file - when I run yarn as instructed by README, it doesn't find yarn.lock, fetches the latest dependencies and generates it from scratch.

@kbtz
Copy link
Author

kbtz commented Feb 21, 2018

@maximbaz ah yeah, that thing... yarn has known issues with post-install scripts and is having a hard time trying to keep up with npm 5, so I'd stay away from that specially when compilation is involved (I can't even install node-sass with it).

Just for the sake of this test I think we can build the new depencies without emojis for a while 😅

@maximbaz
Copy link
Contributor

All in all your fork does fix the compilation issue, thanks!

I'll leave you with @KELiON to decide which tools to use or not to use, just please update README to reflect the "approved" approach - those commands I will use in AUR packages.

@kbtz
Copy link
Author

kbtz commented Feb 22, 2018

@maximbaz Thanks for the confirmation.
Indeed either @KELiON or @TooTallNate will have to take action on this, this PR was created just to hold this discussion.

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