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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

oliverfoster
Copy link

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

@oliverfoster oliverfoster self-assigned this Feb 21, 2024
@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
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
Author

oliverfoster commented Mar 1, 2024 •
edited

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
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
Author

Did you try adding the same script at postupdate?

@oliverfoster
Copy link
Author

Is there any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-steele chris-steele chris-steele approved these changes

@taylortom taylortom Awaiting requested review from taylortom

Assignees

@oliverfoster oliverfoster

Labels
None yet
Projects
adapt-authoring: The TODO Board
Status: Needs Reviewing
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Defining interdependencies between Adapt modules
2 participants
@oliverfoster @chris-steele