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

Exclude Satellite from pnpm workspace #3228

Closed
wants to merge 1 commit into from

Conversation

DukeManh
Copy link
Contributor

@DukeManh DukeManh commented Mar 16, 2022

Exclude Satellite from pnpm workspaces.

I think we misunderstood the purpose of pnpm-workspace.yaml. The file exists to tell pnpm to link packages from the workspace if the available packages match the declared ranges.

So if we do not want to use the local Satellite, for example, we should not list it in pnpm-worksapce.

Better fix of #3191

@gitpod-io
Copy link

gitpod-io bot commented Mar 16, 2022

@DukeManh DukeManh requested review from cindyorangis, menghif, humphd, RC-Lee and joelazwar and removed request for cindyorangis and menghif March 16, 2022 20:51
@DukeManh DukeManh marked this pull request as draft March 16, 2022 20:53
@DukeManh DukeManh changed the title Link 'eslint-config' from workspace Exclude Satellite from pnpm workspace Mar 16, 2022
@DukeManh
Copy link
Contributor Author

I updated the description.

@DukeManh DukeManh marked this pull request as ready for review March 16, 2022 21:35
@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 16, 2022

I thought we didn't want the services to use local Satellite though

@DukeManh
Copy link
Contributor Author

@rclee91, my bad, I updated the description again, s/do/do not/

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 16, 2022

In this case, do we still use pnpm install to get the right dependencies inside of Satellite?

@cindyorangis
Copy link
Contributor

cindyorangis commented Mar 16, 2022

I guess we're giving Satellite the same treatment as our React Native app so they're both living in the same repository as the rest of the services but they functioning like introverted roommates that don't explicitly communicate with the other services unless we want them to by doing this?

<path/to/service>/package.json

"@senecadot/satellite": "workspace:^"

@DukeManh
Copy link
Contributor Author

In this case, do we still use pnpm install to get the right dependencies inside of Satellite?

yes

RC-Lee
RC-Lee previously approved these changes Mar 16, 2022
@RC-Lee RC-Lee self-requested a review March 16, 2022 21:49
@DukeManh
Copy link
Contributor Author

Wait, I'm wrong.

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 16, 2022

In this case, do we still use pnpm install to get the right dependencies inside of Satellite?

yes

Sorry, have you tried deleting all the node_modules and doing pnpm install to see what happens to the node_modules? Including the ones in src/satellite
Taking out Satellite in the pnpm workspace was a part of the "hack" in #3191 , I seem to remember it won't create node_modules in src/satellite
I could try this again, but my potato laptop takes a long time to test. Just wondering if you had tried.

@RC-Lee RC-Lee dismissed their stale review March 16, 2022 21:53

Not approving just yet

@cindyorangis
Copy link
Contributor

We probably have to do the same thing we do with React Native with postinstall,

"postinstall": "cd src/mobile && npm install"

@DukeManh
Copy link
Contributor Author

We probably have to do the same thing we do with React Native with postinstall,

Yes, probably, either we have to figure out how to link Satellite or do that.
We could use the workspace:protocol but it makes Vercel deployment fails here https://github.com/Seneca-CDOT/telescope/runs/5576676664?check_suite_focus=true

@DukeManh
Copy link
Contributor Author

This is probably a bug in pnpm pnpm/pnpm#3561

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 18, 2022

Is this something that is still needed, even after the merge of #3247 ?

@DukeManh
Copy link
Contributor Author

@rclee91 we don't need this anymore. I'm gonna close it.

@DukeManh DukeManh closed this Mar 18, 2022
@DukeManh DukeManh deleted the eslint-config branch March 18, 2022 02:31
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