-
Notifications
You must be signed in to change notification settings - Fork 21
chore(js): decouple workspaces APIC-550 #739
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
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
6d0aeae
to
56b2d02
Compare
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
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.
I understand the motivations but not sure to see the benefits compared to the working solution we have (for now).
However, as we plan to remove generated code from this repo so it’s a great first step!
The benefits is that this remove some of the specific logic for js, and allows to work like any other language (if we didn't cache the node_modules, maybe this could be done dynamically), and it makes sure that our playground and cts is always coupled to the correct api client version, not a random one on npm |
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 benefits is that this remove some of the specific logic for js
I agree, but wanted to know if there was any other compared to the logic in place. Again, it's a first step of removing generated code so it's great
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.
Mostly style comments, feel free to pick
GG :D
@@ -21,11 +21,12 @@ async function buildPerClient( | |||
`yarn workspace ${npmNamespace}/${additionalProperties.packageName} clean`, | |||
{ | |||
verbose, | |||
cwd: getLanguageFolder(language), |
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.
maybe in this file the const cwd = get...
would make calls cleaner but it's a detail
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.
it's in 2 separated functions so it's complicated, I'll leave it like this for now 😃
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.
yup not a big deal
.github/workflows/check.yml
Outdated
steps: | ||
- uses: actions/checkout@v2 | ||
|
||
- name: Cache bundled specs | ||
id: cache | ||
uses: actions/cache@v3 | ||
with: | ||
key: ${{ matrix.client.cacheKey }} | ||
path: ${{ matrix.client.bundledPath }} | ||
key: ${{ fromJSON(needs.setup.outputs.SPECS_MATRIX).cacheKey }} |
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.
we can set an env for the job level e.g. under needs
env:
matrix: ${{ fromJSON(needs.setup.outputs.SPECS_MATRIX) }}
so we can use it everywhere in it :D
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.
no way !
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.
Nice!
@@ -96,15 +96,17 @@ jobs: | |||
runs-on: ubuntu-20.04 | |||
timeout-minutes: 10 | |||
needs: setup | |||
env: | |||
matrix: ${{ fromJSON(needs.setup.outputs.SPECS_MATRIX) }} |
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.
c'est bo hein
🧭 What and Why
🎟 JIRA Ticket: APIC-550
Remove js client and tests from the monorepo workspaces, this allows for more control for when we want to install the deps and should reduce CI times.
Also useful because we don't need to bump the playground and the tests because we use relative path instead of version.
It should make it easier for the release too, there is only one place where we need to run
YARN_ENABLE_IMMUTABLE_INSTALLS=false
(after the generation)It may make things more difficult when editing package.json, you need to make sure to run yarn install at some point
Changes included:
yarn.lock
for js client and js tests🧪 Test
run the playground