This repository has been archived by the owner on Jan 25, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: better cache support for JS workspaces #526
feat: better cache support for JS workspaces #526
Changes from 5 commits
5e24377
620c1ce
4a61af7
ef73c84
70c8a36
32f7b0d
14b1ea8
073a3db
8ab88ae
c4e3641
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sand] Can we clarify a bit what
YARN_IGNORE_PATH
does?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to had a better description. Let me know what you think @erezrokah 👍 The naming for the variable vs its final purpose can be a bit confusing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JGAntunes Are these env vars intended to be user-editable in the long term? If so, we'll need to document them. (cc @rstavchansky)
Is there anything else that should be documented? Or is this automatic behavior that's considered more "under the hood"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JGAntunes - so we are forcing
yarn@1
just for the workspaces command?And this works since the configuration hasn't changed between v1 and v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only new user-editable env var we're introducing is
NETLIFY_YARN_WORKSPACES
, and it shouldn't be editable in the long term, hence no need to document it IMO CC @rstavchansky. Theoretically speaking, users could set an environment variableNETLIFY_YARN_WORKSPACES=true
that would trigger this behaviour (instead of relying on a feature flag on our side to do it - which is our plan to roll this out) but I don't really see that as major concern. The plan is for this behaviour to eventually become default without the need of any env vars (we infer the presence of aworkspace
monorepo on our end).That being said, later on (maybe once this has been rolled out accordingly), I think it would be a good idea to produce some examples of monorepo usages with Netlify as it is not a trivial setup to do and, to my understanding, we don't have a write up on this (correct me if I'm wrong though). I think there's a lot of workarounds that originated in the issues above that are no longer applicable, so documenting that could probably be beneficial. Happy to move this discussion back to - https://github.com/netlify/pod-workflow/issues/139 - if you prefer 👍 CC @rstavchansky @verythorough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly @erezrokah! The configuration is still extracted from the
workspaces
entry in thepackage.json
and that hasn't changed (from what I managed to look into), that way we are able to validate the presence ofworkspaces
without the need of a separate branch for theyarn berry
case.Is that clear enough from the comment and usage? I'm happy to provide further information in there if you think like it makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YARN_IGNORE_PATH=1 yarn workspaces info
is called in the test above.Would it make sense to make the test use
--json
and keep the output, to avoid callingyarn workspaces info
twice? The test would then check exit code using$?
. As a tiny performance optimization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed via 073a3db 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, actually fixed via 8ab88ae 😅
Btw, I'm keeping the exit code in a local variable as a way to avoid hitting - https://github.com/koalaman/shellcheck/wiki/SC2181 - let me know what you think @ehmicky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to directly set
NETLIFY_JS_WORKSPACE_LOCATIONS
instead ofpackage_locations
? This might require declaringNETLIFY_JS_WORKSPACE_LOCATIONS
as a file-level array variable. This would remove the need for the intermediary variablepackage_locations
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the cache logic and the respective usage of
NETLIFY_JS_WORKSPACE_LOCATIONS
is kind of isolated from therun_yarn
function, I would say there's some use in keeping it that way? 🤔 That way we're keeping all the convoluted logic required to cacheworkspaces
in the respectiverestore_js_workspaces_cache
,cache_js_workspaces
andcache_node_modules
. The only thing therun_yarn
is doing right is to pass thelocations
to the underlying functions ensuring it's decoupled from that. My concern is that if we end up spreading theNETLIFY_JS_WORKSPACE_LOCATIONS
variable around we might end up with a tighter spaghetti bowl 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to, once we can drop the feature flag, we keep only the
cache_node_modules
function containing all the logic relative tonode_modules
caching.