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

chore: fix package link script issues #6251

Merged
merged 2 commits into from
May 27, 2020
Merged

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented May 20, 2020

Resolves #6250
Impact: minor
Type: chore

  • npm install official packages after unlink
  • don't unlink unless it is truly linked
  • require package namespace for both commands

Breaking changes

The package-link and package-unlink commands now need the full package name, including @reactioncommerce/ org prefix.

Testing

Try it

- npm install official packages after unlink
- don't unlink unless it is truly linked
- require package namespace for both commands

Signed-off-by: Eric Dobbertin <[email protected]>
@aldeed
Copy link
Contributor Author

aldeed commented May 20, 2020

I'd like to also figure out a way to maintain the other links when unlinking one, but I think we'd have to create a file to track the links and re-create them all in the unlink script after unlinking. In other words, essentially write our own version of the NPM RFC proposal.

A bigger concern is that I am seeing that often, after I link a second package, the code from the first package is no longer executing, even though the symlink is still there and the symlinked files have the changes. It's like there's a different version of the package also installed somewhere in the node_modules tree, and the import() is finding that one instead. So far I haven't figured out what's going on.

@aldeed aldeed marked this pull request as ready for review May 21, 2020 17:32
@aldeed
Copy link
Contributor Author

aldeed commented May 21, 2020

My "bigger concern" comment is captured in #6250, and I think I actually fixed it by changing docker cp command in this PR. So I believe the only remaining issue is that unlink unlinks all packages and then you have to relink any you still want linked. Most likely will not figure out a solution for that in this PR.

@aldeed aldeed requested a review from kieckhafer May 21, 2020 17:34
@aldeed aldeed removed the request for review from kieckhafer May 22, 2020 15:05
@focusaurus
Copy link
Contributor

OK I think I encountered some of what is being discussed in #6250 today so I'm going to test this branch and see if it helps. One thing I'd like to confirm about this tooling, is it:

  • use BOTH package-link and a docker volume mount

or is this package-link workflow designed to work without an extra docker volume mount for the api-plugin you're working on?

@kieckhafer
Copy link
Member

kieckhafer commented May 22, 2020

OK I think I encountered some of what is being discussed in #6250 today so I'm going to test this branch and see if it helps. One thing I'd like to confirm about this tooling, is it:

  • use BOTH package-link and a docker volume mount

or is this package-link workflow designed to work without an extra docker volume mount for the api-plugin you're working on?

When testing originally, I did not have to add the additional volume mount. I followed the example workflow here and was able to link and unlink: #6238 I only did a single time, so did not run into the issues of #6250

@kieckhafer
Copy link
Member

kieckhafer commented May 22, 2020

I’m going to hold off on the 3.8.0 release until this is merged. The only big change in that release is the original PR which this current PR is fixing, so makes sense to wait. cc @mdelreal

@kieckhafer kieckhafer mentioned this pull request May 22, 2020
@loan-laux
Copy link
Collaborator

I gave it another try with @aldeed's latest changes and didn't run into the issues mentioned in #6250 anymore. Been using bin/package-link for the whole day and it seems very stable. On my end, seems like this is good to go.

@kieckhafer
Copy link
Member

I've also tested this out and it seems to be good. @focusaurus have you had a chance to try it out?

@focusaurus
Copy link
Contributor

Yeah I've used this a few times. I'm still not 100% clear on the workflow and the various levels of technical challenges and solutions that are in play here, but it seems to work OK for me.

@focusaurus focusaurus merged commit 0789bd3 into trunk May 27, 2020
@focusaurus focusaurus deleted the fix-package-link-issues branch May 27, 2020 16:45
@kieckhafer kieckhafer mentioned this pull request Aug 4, 2020
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.

Linked packages don't get updated with new changes
4 participants