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

Fix: Clear up workspace subdependencies (fixes #629) #630

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

oliverfoster
Copy link
Contributor

fixes #629

Allows workspaces to use giturl dependencies and peer dependencies by cleaning up erroneously installed duplicates of the workspace modules.

Fix

  • Adds postinstall script to clean up workspace duplicate sub dependencies

Test

git clone https://github.com/adapt-security/adapt-authoring 629-workspacesubdependencies
cd 629-workspacesubdependencies
git checkout issue/629
git clone https://github.com/adapt-security/adapt-authoring-contentplugin local_adapt_modules/adapt-authoring-contentplugin
git clone https://github.com/adapt-security/adapt-authoring-core local_adapt_modules/adapt-authoring-core
npm install --force # to override peer dependency resolve errors

image

@chris-steele
Copy link

First pass was excellent @oliverfoster :

  • started with clean clone of adapt-authoring
  • placed adaptframework, api, content and contentplugin clones in workspace
  • ran npm i --force

Post intall script removed erroneous sub deps in workspaces and also in root modules (coursetheme and mongodblogger)

Ran node ./bin/start.js - tool built and started with no issues 💯

@oliverfoster
Copy link
Contributor Author

This will all hopefully be fixed by itself for the most part once the repos are registered on npm. This is as the subdependencies when defined with "name": "version" should resolve against workspace modules properly where they don't with "name": "giturl".

@chris-steele
Copy link

@oliverfoster is it possible to have this run after npm update? I just realised after running that command all the erroneous installs return.

@oliverfoster
Copy link
Contributor Author

oliverfoster commented Mar 1, 2024

Probably. Although it may be that you just need to run npm install to do updates.

Here's the documentation for how scripts are named with pre and post.

https://docs.npmjs.com/cli/v10/using-npm/scripts

And the update command.

https://docs.npmjs.com/cli/v10/commands/npm-update

You could also run it manually with npm run postinstall

@chris-steele
Copy link

Great tips thank you 💯

@oliverfoster
Copy link
Contributor Author

@chris-steele did you find a way to get it to work on npm update?

@chris-steele
Copy link

Yes @oliverfoster I can run npm run postinstall after npm update and it works as expected 👍

@oliverfoster
Copy link
Contributor Author

Did you try adding the same script at postupdate?

@oliverfoster
Copy link
Contributor Author

Is there any progress on this?

@taylortom taylortom added the high priority Should be prioritised over all other issues label Jun 19, 2024
@chris-steele
Copy link

I did try postupdate, but I don't think it is available with npm

@oliverfoster
Copy link
Contributor Author

oliverfoster commented Sep 2, 2024

It is not, correct.

npm/rfcs#226

@chris-steele
Copy link

Looks to have fizzled out and died a death

@taylortom taylortom merged commit fadb614 into master Sep 5, 2024
4 checks passed
@taylortom taylortom deleted the issue/629 branch September 5, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Should be prioritised over all other issues
Projects
Development

Successfully merging this pull request may close these issues.

Defining interdependencies between Adapt modules
3 participants