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

Refactor to reference glide-rs as a separate external package - Yarn fix #2943

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Muhammad-awawdi-amazon
Copy link
Collaborator

@Muhammad-awawdi-amazon Muhammad-awawdi-amazon commented Jan 12, 2025

Description

Yarn interprets the 'file:rust-client' path relative to the user's working directory instead of the package's actual location, which causes the dependency resolution to fail.

Desired Architecture

Base Package (Node.js Code)

  • Should be published as a standalone package

  • Contains platform-independent Node.js code

  • Only needs to be published once since it's not architecture-specific

  • Serves as the main entry point for users

Rust Client Package

  • Should be published separately for each platform/architecture

  • Contains the architecture-specific binaries

  • Needs multiple versions:

  • One for each supported operating system (Linux, MacOS, etc.)

  • One for each supported CPU architecture (x64, arm64, etc.)

  • Includes its own package.json for version management

  • Gets published multiple times to support different platforms

Issue link

This Pull Request is linked to issue (URL): #2680

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.

@Muhammad-awawdi-amazon Muhammad-awawdi-amazon added node Node.js wrapper CI/CD CI/CD related Users Pain An issue known to cause users pain, generaly open by the user. User issue Issue openned by users labels Jan 12, 2025
@Muhammad-awawdi-amazon Muhammad-awawdi-amazon requested a review from a team as a code owner January 12, 2025 13:01
@Muhammad-awawdi-amazon Muhammad-awawdi-amazon changed the base branch from yarn-fix to main January 12, 2025 13:02
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

LGTM

Could you please document your changes by updating PR description. What packages are published, what dependency do they have?
Consider adding an example app which uses yarn.

Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD CI/CD related node Node.js wrapper User issue Issue openned by users Users Pain An issue known to cause users pain, generaly open by the user.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants