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

Update all modules' peerDependencies #493

Open
taylortom opened this issue Feb 14, 2022 · 3 comments
Open

Update all modules' peerDependencies #493

taylortom opened this issue Feb 14, 2022 · 3 comments
Labels
dependencies Pull requests that update a dependency file high priority Should be prioritised over all other issues question Discussion is required standards Related to coding standards
Milestone

Comments

@taylortom
Copy link
Collaborator

taylortom commented Feb 14, 2022

Need to make sure all module interdependencies are defined using the peerDependencies object.

This includes the following:

  • ESM imports
  • Modules imported using waitForModule

See this article for a bit of an explanation.

@taylortom taylortom added help wanted standards Related to coding standards dependencies Pull requests that update a dependency file labels Feb 14, 2022
@taylortom taylortom added this to the v1.0.0 milestone Feb 14, 2022
@taylortom taylortom modified the milestones: v1.0.0-rc.1, v1.0.0 Aug 9, 2022
@taylortom taylortom moved this from New to Backlog in adapt-authoring: The TODO Board Feb 23, 2023
@taylortom taylortom added question Discussion is required and removed help wanted labels Feb 23, 2023
@taylortom taylortom moved this from Backlog to Awaiting Response in adapt-authoring: The TODO Board Feb 23, 2023
@chris-steele chris-steele self-assigned this Jan 23, 2024
chris-steele added a commit to adapt-security/adapt-authoring-adaptframework that referenced this issue Jan 23, 2024
chris-steele added a commit to adapt-security/adapt-authoring-api that referenced this issue Jan 23, 2024
chris-steele added a commit to adapt-security/adapt-authoring-assets that referenced this issue Jan 23, 2024
@chris-steele
Copy link

chris-steele commented Jan 29, 2024

@taylortom I've taken down the comments I added to make some revisions - will post back soon.

@chris-steele
Copy link

chris-steele commented Feb 6, 2024

UPDATE on the below comments: at present I am correcting dependencies across packages and adding peerDependencies as per OP to verify Node behaviour (especially w.r.t. workspaces).

Peer dependencies are for plugin architectures. The AAT doesn't implement a plugin architecture: it is a single application split across packages. These packages have a mix of import and runtime interdependencies. Most packages are therefore coupled and some tightly (jsonschema <-> config, content <-> contentplugin).

Propose the following:

  1. drop the notion of peer dependencies completely
  2. remove all adapt-* dependencies from package.json files and hoist them to the root project package.json (adapt-authoring)

This will mean we have a single place to change paths (e.g. select a different Git branch) for whatever dependencies we like and therefore not have resolution conflicts when we are using Node workspaces.

For example, if I am working on adaptframework issue/31, and content issue/23 (but from a different Git user) I can just edit one package.json with:

"adapt-authoring-adaptframework": "github:adapt-security/adapt-authoring-adaptframework#issue/31",
"adapt-authoring-content": "github:chris-steele/adapt-authoring-content#issue/23"

@chris-steele chris-steele moved this from Assigned to Needs Reviewing in adapt-authoring: The TODO Board Feb 6, 2024
@chris-steele chris-steele removed their assignment Feb 6, 2024
@taylortom
Copy link
Collaborator Author

As discussed with Ollie, we'll keep nested dependencies to allow for simpler installations and a more transparent dependency map.

After running a few tests on peer dependencies, I agree that they introduce more problems than they solve, so think we need to forget that approach for now. I do still think we need a way for developers to flag when certain modules are used at runtime to give useful warnings to the user.

@taylortom taylortom moved this from Needs Reviewing to Backlog in adapt-authoring: The TODO Board Jun 18, 2024
@taylortom taylortom added the high priority Should be prioritised over all other issues label Jun 19, 2024
@taylortom taylortom moved this from Backlog to Awaiting Response in adapt-authoring: The TODO Board Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file high priority Should be prioritised over all other issues question Discussion is required standards Related to coding standards
Projects
Status: Awaiting Response
Development

No branches or pull requests

2 participants