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

Update developer documentation: yalc, development workflow #147

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

lyzadanger
Copy link
Contributor

Update development guide with additions about workflow; update section
on how to remove a yalc-linked package from an application project.

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #147 (a550fce) into main (b89b25b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #147   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          149       149           
  Branches        51        51           
=========================================
  Hits           149       149           

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 b89b25b...a550fce. Read the comment docs.

# from the client dir
$ yarn --check-files
$ cd <application project directory>
$ yalc remove @hypothesis/frontend-shared
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most salient change. I'm not convinced yarn --check-files does what this intends; I am more certain that yalc remove does.

Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like yalc remove and yarn --check-files do different things. yalc remove removes the copy of the package in the project's .yalc dir, but does't appear to update the contents of node_modules/, which is what matters most. yarn --check-files ensures that the contents of node_modules/ matches yarn.lock (ie. reset all files to published packages from npm) but doesn't touch the .yalc directory.

In other words, it doesn't look like yalc remove "restores the project-defined package version".

# Initial state - `@hypothesis/frontend-shared` installed from npm
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
drwxr-xr-x  8 robert  staff   256B 26 Jul 08:52 frontend-shared

# After running `yarn link`, `node_modules/hypothesis-shared` becomes a symlink
$ yalc link @hypothesis/frontend-shared
Package @hypothesis/frontend-shared@3.6.0+c970582d linked ==> /Users/robert/hypothesis/client/node_modules/@hypothesis/frontend-shared
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
lrwxr-xr-x  1 robert  staff    65B 26 Jul 08:52 frontend-shared -> /Users/robert/hypothesis/client/.yalc/@hypothesis/frontend-shared
$ ls -lh .yalc/@hypothesis/
total 0
drwxr-xr-x  9 robert  staff   288B 26 Jul 08:52 frontend-shared

# Running `yalc remove` removes the copy from `.yalic/`, but doesn't update the symlink in `node_modules/`
$ yalc remove @hypothesis/frontend-shared
Removing installation of @hypothesis/frontend-shared in /Users/robert/hypothesis/client
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
lrwxr-xr-x  1 robert  staff    65B 26 Jul 08:52 frontend-shared -> /Users/robert/hypothesis/client/.yalc/@hypothesis/frontend-shared
$ ls -lh .yalc/

# Running `yarn --check-files` does replace the symlink, but doesn't touch any files in `.yalc`.
$ yarn --check-files
yarn install v1.22.10
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...

✨  Done in 14.32s.
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
drwxr-xr-x  8 robert  staff   256B 26 Jul 08:53 frontend-shared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks to me like yalc remove and yarn --check-files do different things. yalc remove removes the copy of the package in the project's .yalc dir, but does't appear to update the contents of node_modules/, which is what matters most. yarn --check-files ensures that the contents of node_modules/ matches yarn.lock (ie. reset all files to published packages from npm) but doesn't touch the .yalc directory.

In other words, it doesn't look like yalc remove "restores the project-defined package version".

I wish yalc's docs were a little clearer on these points (I know, I'm casting blame around a bit here).

Let's say the goal in a given local project repo (e.g. lms, client) is to make sure that project is using the package version for @hypothesis/frontend-shared as defined by yarn.lock (or package.json, if need be), and that all .yalc metadata is cleaned up. i.e. "Restore this project's dependencies, don't yalc-ify anything".

To kind of regurgitate what I think you've written here: Sounds like yarn --check-files will "actually update the node_modules contents" to the package version in yarn.lock, but that leaves some .yalc floating around. yalc remove removes that .yalc metadata. Correct?

In which case, is it necessary, perhaps, to use both commands to fully clean up?

Copy link
Member

Choose a reason for hiding this comment

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

In which case, is it necessary, perhaps, to use both commands to fully clean up?

Yes. However assuming that the developer only really cares about updating node_module to match yarn.lock, yarn --check-files is the only command of the two that is essential.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

In my testing with the latest version of yalc (1.0.0-pre.53), yalc remove doesn't do what the README says. It removes the copy of the package from the consuming project's .yalc directory, but doesn't update node_modules/@hypothesis/frontend-shared to match the version specified in package.json/yarn.lock.

The other changes in this PR LGTM.


Build the package and then publish to your local yalc repository.
This guide explains how to test other applications (e.g. [`client`](https://github.com/hypothesis/client), `lms`) with
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This guide explains how to test other applications (e.g. [`client`](https://github.com/hypothesis/client), `lms`) with
This section explains how to test other applications (e.g. [`client`](https://github.com/hypothesis/client), `lms`) with


If you wish to revert back to the current installed version of `frontend-shared`, simply run:
Remove `yalc` linking from an application and restore the project-defined
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Remove `yalc` linking from an application and restore the project-defined
To remove `yalc` linking from an application and restore the project-defined

# from the client dir
$ yarn --check-files
$ cd <application project directory>
$ yalc remove @hypothesis/frontend-shared
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like yalc remove and yarn --check-files do different things. yalc remove removes the copy of the package in the project's .yalc dir, but does't appear to update the contents of node_modules/, which is what matters most. yarn --check-files ensures that the contents of node_modules/ matches yarn.lock (ie. reset all files to published packages from npm) but doesn't touch the .yalc directory.

In other words, it doesn't look like yalc remove "restores the project-defined package version".

# Initial state - `@hypothesis/frontend-shared` installed from npm
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
drwxr-xr-x  8 robert  staff   256B 26 Jul 08:52 frontend-shared

# After running `yarn link`, `node_modules/hypothesis-shared` becomes a symlink
$ yalc link @hypothesis/frontend-shared
Package @hypothesis/frontend-shared@3.6.0+c970582d linked ==> /Users/robert/hypothesis/client/node_modules/@hypothesis/frontend-shared
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
lrwxr-xr-x  1 robert  staff    65B 26 Jul 08:52 frontend-shared -> /Users/robert/hypothesis/client/.yalc/@hypothesis/frontend-shared
$ ls -lh .yalc/@hypothesis/
total 0
drwxr-xr-x  9 robert  staff   288B 26 Jul 08:52 frontend-shared

# Running `yalc remove` removes the copy from `.yalic/`, but doesn't update the symlink in `node_modules/`
$ yalc remove @hypothesis/frontend-shared
Removing installation of @hypothesis/frontend-shared in /Users/robert/hypothesis/client
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
lrwxr-xr-x  1 robert  staff    65B 26 Jul 08:52 frontend-shared -> /Users/robert/hypothesis/client/.yalc/@hypothesis/frontend-shared
$ ls -lh .yalc/

# Running `yarn --check-files` does replace the symlink, but doesn't touch any files in `.yalc`.
$ yarn --check-files
yarn install v1.22.10
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...

✨  Done in 14.32s.
$ ls -lh node_modules/@hypothesis/ | grep frontend-shared
drwxr-xr-x  8 robert  staff   256B 26 Jul 08:53 frontend-shared

Update development guide with additions about workflow; update section
on how to remove a yalc-linked package from an application project.
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM

@lyzadanger lyzadanger merged commit cde15a2 into main Aug 5, 2021
@lyzadanger lyzadanger deleted the update-dev-docs branch August 5, 2021 16:22
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