Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use yarn 3.6.3 #788

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use yarn 3.6.3 #788

wants to merge 4 commits into from

Conversation

wch
Copy link
Collaborator

@wch wch commented Sep 6, 2023

bslib currently uses whichever version of yarn is installed on the system. This PR updates it to use a fixed version of yarn, so that the behavior is predictable across systems. (This is how things are set up for the https://github.com/rstudio/shiny repository.)

@wch wch requested review from cpsievert and gadenbuie September 6, 2023 19:38
@gadenbuie
Copy link
Member

@cpsievert Do you know if this --production flag is needed?

with_dir("inst", system("yarn install --production"))

We can apparently get similar behavior using yarn workspaces focus --production, but that requires the workspaces plugin and https://yarnpkg.com/cli/workspaces/focus seems to indicate that it's marginally useful.

@cpsievert
Copy link
Collaborator

Do you know if this --production flag is needed?

IIRC it's there so we can essentially rename inst/node_modules to inst/lib without having to explicitly pick which dependencies to keep. If we can do that in a smarter way, I'd definitely be willing to consider another approach

Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

After spending a bit trying to get this to work, I don't think it's currently worth the effort, unless someone else has an easy solution for asking yarn to install just the development dependencies and have them installed into inst/node_modules reliably.

In short, this PR moves bslib from classic yarn to modern yarn, which has more downstream changes than I think we want to take on right now. I think we should table this for now and revisit this post-conf.

@wch
Copy link
Collaborator Author

wch commented Sep 6, 2023

The problem I had was that when I ran yarn (version 1.22.17), it errored out because it apparently didn't know how to get the commit of rstudio/shiny specified in yarn.lock. I managed to get it working locally with the change in this PR, but I'm fine with waiting until post-conf before we merge it.

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

Successfully merging this pull request may close these issues.

3 participants