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

Yarn/pnpm/workspace-support: Make export work #86

Merged
merged 7 commits into from
Jan 26, 2023
Merged

Conversation

jim-lub
Copy link
Contributor

@jim-lub jim-lub commented Nov 22, 2022

  • We now also search for a yarn.lock file
  • If we can't find a lock file in the server directory we go up a maximum of 5 levels to find it. This allows the export to be used in a workspaces setup.

@jim-lub jim-lub requested a review from kamilafsar November 22, 2022 06:15
@JasperH8g
Copy link
Contributor

JasperH8g commented Jan 16, 2023

Great starting-point of this problem @jim-lub ! I do think traversing up the users disk is too tricky to do, since we don’t know if there’s an unrelated lock-file present outside of the project-dir of the user.

I’ve done some research and it seems to be possible to get information about workspaces, which we can use to figure out the root of the project. We can use this to get the exact location of the lock-file in the root of it.

Doing this did raise the question though: While we’re looking for the lock-file in the root, shouldn’t we be looking for the lock-file for the server specifically? In workspaces this isn’t present, but is there a way to generate it at will? It seems that we’re not alone: yarnpkg/yarn#4521. As I can figure out atm, this isn’t possible (yet), but using the lock-file in the root seems to be the correct approach after all: yarnpkg/yarn#4521 (comment).

I’ll keep digging a bit, and otherwise use the yarn-cli to get to the root of the repo and get the lock-file from there. TBC ☺️

Edit: I’ll investigate this approach as well: yarnpkg/yarn#4521 (comment) => Doesn't seem ideal, because we'll need to configure an offline-mirror just for this.

@JasperH8g JasperH8g changed the title Add export support for yarn and workspaces Add export support for yarn/npm/pnpm workspaces Jan 17, 2023
@JasperH8g
Copy link
Contributor

First of all, I've added the pnpm-lockfile as well 😊

Also, after some internal discussion with @jim-lub and @kamilafsar, we've concluded that it's allright to traverse up the directory-structure (it's better than having dependencies on multiple package-managers anyway), but let's make it a bit more secure, because we'll prevent leaving the project-root as much as possible:

  • Step 1: Search for a lock-file (npm/pnpm/yarn) in the current directory. If that's found, all is good and done.
  • Step 2: Traverse one level up, find out if there are workspace-settings (npm/pnpm/yarn) available, get the lock-file (npm/pnpm/yarn) from that level. If there're no such settings, go a level higher and so on.

I'll dive into implementing these changes in the upcoming days 👍

@JasperH8g
Copy link
Contributor

d90d975 should do it, I'll get into testing this extensively soon 😊

@JasperH8g
Copy link
Contributor

So we won't forget, this should be fixing #88 🙂

@JasperH8g JasperH8g changed the title Add export support for yarn/npm/pnpm workspaces Yarn/pnpm/workspace-support: Make export work Jan 19, 2023
Copy link
Contributor Author

@jim-lub jim-lub left a comment

Choose a reason for hiding this comment

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

Looks good. If we create a RC I can test this out in my yarn workspace somewhere in the next few days.

packages/server/src/commands/export/domain.ts Show resolved Hide resolved
Copy link
Contributor

@kamilafsar kamilafsar left a comment

Choose a reason for hiding this comment

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

Seems solid. I'd like to make the error a bit more descriptive as "lockfile" without context could be a bit confusing. The other stuff you can threat it as recommendation for better readability :)

packages/server/src/commands/export/index.ts Outdated Show resolved Hide resolved
packages/server/src/commands/export/index.ts Outdated Show resolved Hide resolved
packages/server/src/commands/export/index.ts Show resolved Hide resolved
@JasperH8g JasperH8g merged commit f6fa7a5 into main Jan 26, 2023
@JasperH8g JasperH8g deleted the fix/export-lock-file branch January 26, 2023 14:01
@JasperH8g
Copy link
Contributor

JasperH8g commented Jan 26, 2023

This is released in @phero/[email protected]. We'll do some testing before releasing it as latest.

To test it yourself, run:

npm uninstall @phero/server && npm install @phero/server@next
npx phero@next

@JasperH8g JasperH8g mentioned this pull request Jan 26, 2023
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.

None yet

3 participants