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

[AIRFLOW-6272] Switch from npm to yarnpkg for managing front-end dependencies #6844

Merged
merged 2 commits into from
Dec 19, 2019

Conversation

ashb
Copy link
Member

@ashb ashb commented Dec 18, 2019

Make sure you have checked all steps below.

Jira

Description

It is:

  • quicker to install
  • easier to get repeatable results
  • Takes up less space (130MB/15k files vs 190MB/23k files)
  • nicer to user (has better help)

It is:

- quicker to install
- easier to get repeatable results
- Takes up less space (130MB/15k files vs 190MB/23k files)
- nicer to user (has better help)
@ashb ashb requested review from potiuk and mik-laj December 18, 2019 21:54
@ashb
Copy link
Member Author

ashb commented Dec 18, 2019

No real reason we have to switch, I just find yarn nicer to use

@potiuk
Copy link
Member

potiuk commented Dec 18, 2019

Just a question - if we are going to change - why don't we change to pnpm instead. It seems it's relatively new but stable/recommended package managers for node. And I like the output of it even more than yarn's. Also it shares downloaded packages via symbolic/hard links to .pnpm directory thus saving space.

It also has saner (than npm) commands (similar to yarn):

pnpm install --frozen-lockfile

rather than

npm ci

It is also quite significantly faster. I installed all packages for airflow with npm, pnpm and yarn I got (with cache removed):

time pnpm install --frozen-lockfile
...
real	1m10.994s
user	0m11.005s
sys	0m19.809s

du -s -h node_modules/
146M	node_modules/
time yarn install --frozen-lockfile
...
real	2m47.795s
user	0m12.539s
sys	0m16.754s

du -s -h node_modules/
178M	node_modules/

I could not check npm in the same way (as the latest npm fails installing our packages on my mac currently. But I think the savings are significant pnpm vs. yarn.

@potiuk
Copy link
Member

potiuk commented Dec 18, 2019

Also it seems pnpm has a nicer way of handling accidentally installed packages: https://medium.com/pnpm/pnpms-strictness-helps-to-avoid-silly-bugs-9a15fb306308

@ashb
Copy link
Member Author

ashb commented Dec 19, 2019

We could, I'll check it out - my first thing though is that pnpm is mostly the work of one person, where as yarn is backed by facebook. Don't know if that is an issue or not.

@ashb
Copy link
Member Author

ashb commented Dec 19, 2019

Actually one reason I think to use yarn over pnpm -- yarn will detect and resolve conflicts in the lock file, and from what I can see pnpm won't.

@potiuk
Copy link
Member

potiuk commented Dec 19, 2019

Yeah. I have no strong preference here - I usually do not touch those parts. But the arguments with community and conflict resolution are valid. There is this issue still unresolved about conflict resolution in pnpm pnpm/pnpm#2036. And since we are going to do some UI revamp shortly, conflicts might happen more frequently (they are not a problem now when we do not work on UI actively.

I am happy to rely on your choice and use yarn.

@ashb ashb changed the title [AIRFLOW-6272] Switch to yarnpkg for managing front-end dependencies [AIRFLOW-6272] Switch form npm to yarnpkg for managing front-end dependencies Dec 19, 2019
@ashb
Copy link
Member Author

ashb commented Dec 19, 2019

We have more experience with Yarn so let's stick with that.

@potiuk
Copy link
Member

potiuk commented Dec 19, 2019

Fine with me :)

@codecov-io
Copy link

codecov-io commented Dec 19, 2019

Codecov Report

Merging #6844 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6844      +/-   ##
==========================================
+ Coverage   84.49%   84.56%   +0.06%     
==========================================
  Files         680      680              
  Lines       38392    38451      +59     
==========================================
+ Hits        32441    32517      +76     
+ Misses       5951     5934      -17
Impacted Files Coverage Δ
airflow/hooks/presto_hook.py 75.92% <0%> (-1.63%) ⬇️
...low/contrib/operators/datastore_import_operator.py 100% <0%> (ø) ⬆️
airflow/gcp/example_dags/example_datastore.py 100% <0%> (ø) ⬆️
airflow/gcp/example_dags/example_dataflow.py 93.33% <0%> (ø) ⬆️
airflow/contrib/operators/dataflow_operator.py 100% <0%> (ø) ⬆️
...low/contrib/operators/datastore_export_operator.py 100% <0%> (ø) ⬆️
airflow/contrib/hooks/gcp_text_to_speech_hook.py 100% <0%> (ø) ⬆️
airflow/gcp/example_dags/example_bigquery_dts.py 100% <0%> (ø) ⬆️
airflow/contrib/sensors/bigquery_sensor.py 100% <0%> (ø) ⬆️
...w/contrib/operators/gcp_text_to_speech_operator.py 100% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 136dead...002a7bf. Read the comment docs.

@potiuk potiuk changed the title [AIRFLOW-6272] Switch form npm to yarnpkg for managing front-end dependencies [AIRFLOW-6272] Switch fromm npm to yarnpkg for managing front-end dependencies Dec 19, 2019
@potiuk potiuk changed the title [AIRFLOW-6272] Switch fromm npm to yarnpkg for managing front-end dependencies [AIRFLOW-6272] Switch from npm to yarnpkg for managing front-end dependencies Dec 19, 2019
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

One small change to the documentation.

Dockerfile Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
Co-Authored-By: Jarek Potiuk <[email protected]>
@potiuk potiuk merged commit 2a157e3 into apache:master Dec 19, 2019
@ashb ashb deleted the switch-to-yarn branch December 19, 2019 22:01
KKcorps pushed a commit to KKcorps/airflow that referenced this pull request Dec 21, 2019
…ndencies (apache#6844)

It is:

- quicker to install
- easier to get repeatable results
- Takes up less space (130MB/15k files vs 190MB/23k files)
- nicer to user (has better help)
potiuk pushed a commit that referenced this pull request Dec 26, 2019
…ndencies (#6844)

It is:

- quicker to install
- easier to get repeatable results
- Takes up less space (130MB/15k files vs 190MB/23k files)
- nicer to user (has better help)

(cherry picked from commit 2a157e3)
ashb added a commit to astronomer/airflow that referenced this pull request Feb 12, 2020
…ndencies (apache#6844)

It is:

- quicker to install
- easier to get repeatable results
- Takes up less space (130MB/15k files vs 190MB/23k files)
- nicer to user (has better help)

(cherry picked from commit 2a157e3)
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
…ndencies (apache#6844)

It is:

- quicker to install
- easier to get repeatable results
- Takes up less space (130MB/15k files vs 190MB/23k files)
- nicer to user (has better help)
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.

3 participants