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

ARROW-12239: [JS] Switch to yarn #9918

Closed
wants to merge 10 commits into from
Closed

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Apr 7, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Apr 7, 2021

js/.npmrc Outdated Show resolved Hide resolved
@trxcllnt
Copy link
Contributor

trxcllnt commented Apr 7, 2021

While I personally love yarn and prefer it over npm, I think we've stuck to using npm for two reasons:

  1. It ships with node
  2. npm audit and related tooling integration

Do tools like GH security alerts work with yarn.lock files now too? If so, 2 may be less of a concern.

@domoritz
Copy link
Member Author

domoritz commented Apr 7, 2021

Do tools like GH security alerts work with yarn.lock files now too?

Yes, I have seen it with my projects and I only use yarn.

npm audit

There is yarn audit.

@domoritz domoritz requested a review from trxcllnt April 7, 2021 18:53
@domoritz
Copy link
Member Author

domoritz commented Apr 8, 2021

The failures look unrelated to my changes. Do you agree @trxcllnt?

@kou
Copy link
Member

kou commented Apr 8, 2021

It seems that the following error is related to this:

https://github.com/apache/arrow/pull/9918/checks?check_run_id=2298651927#step:7:367

error Your lockfile needs to be updated, but yarn was run with `--frozen-lockfile`.

@domoritz
Copy link
Member Author

domoritz commented Apr 8, 2021

@kou thanks! That came from a merge. I updated the lockfile.

@domoritz
Copy link
Member Author

domoritz commented Apr 8, 2021

@kou There is an error CMake Error: Unknown argument -isystem but I don't see how it's related. Any ideas?

@kou
Copy link
Member

kou commented Apr 9, 2021

It seems that the -isystem error isn't occurred on master.
Can I rebase this branch on master?

@domoritz
Copy link
Member Author

domoritz commented Apr 9, 2021

Go for it. You can also merge since we probably should squash this pull request anyway.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

I've fixed the integration test error.

@kou kou closed this in 113a515 Apr 9, 2021
@domoritz domoritz deleted the yarn branch April 9, 2021 03:54
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
Closes apache#9918 from domoritz/yarn

Lead-authored-by: Dominik Moritz <domoritz@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 10, 2021
Closes apache#9918 from domoritz/yarn

Lead-authored-by: Dominik Moritz <domoritz@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
Closes apache#9918 from domoritz/yarn

Lead-authored-by: Dominik Moritz <domoritz@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@asfimport asfimport mentioned this pull request Apr 9, 2021
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.

None yet

3 participants