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

Handle shared dependencies being required by server dependencies #80

Merged
merged 12 commits into from
Jun 1, 2023

Conversation

ddavness
Copy link
Contributor

@ddavness ddavness commented Apr 3, 2022

Not really the best title to this PR. Fixes #66

@ddavness
Copy link
Contributor Author

ddavness commented Apr 4, 2022

Note to self - when developing on Windows make sure that the encoding of files isn't set to UTF16-LE 😬

@ddavness
Copy link
Contributor Author

ddavness commented Apr 4, 2022

At this point two integration tests were added:

  • cross-realm-dependency - A shared project which requires a server dependency A which in turn requires a shared dependency B. It is expected to FAIL on v0.3.1

  • cross-realm-explicit-dependency - A shared project which explicitly requires A (server) and B (shared) - A also requires B. It PASSES on v0.3.1

rather than it's request realm
When activating a dependency, always use the
origin realm (i.e. inherited realm from state) instead
of it's declared realm
@ddavness ddavness marked this pull request as ready for review April 6, 2022 16:09
@ddavness
Copy link
Contributor Author

ddavness commented Apr 6, 2022

In a nutshell (I think):

  • The resolution process will now tag all dependencies of a server dependency as belonging to the server realm of the project UNLESS one or more of these dependencies is required by a dependency on the shared realm - in that case those dependencies will be moved to the shared realm instead;
  • One unit test was fixed to verify the behavior, and another one was added as a counter-example;
  • Two integration tests were added to confirm the behavior;
  • One of the tests is failing at the moment because the lockfile implementation doesn't seem complete yet;

Copy link
Member

@magnalite magnalite left a comment

Choose a reason for hiding this comment

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

Amazing work on this! I have no comments and agree with all your changes. Lockfiles are now generated correctly (#130) so this is ready to be merged! I have updated the snap files to respect these full lockfiles correctly. Thanks for putting this together and sorry it took so long to merge.

@magnalite magnalite merged commit 76b5172 into UpliftGames:main Jun 1, 2023
@magnalite magnalite added this to the Essential feature complete milestone Jun 13, 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.

2 participants