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

Defining interdependencies between Adapt modules #629

Open
taylortom opened this issue Feb 6, 2024 · 2 comments · May be fixed by #630
Open

Defining interdependencies between Adapt modules #629

taylortom opened this issue Feb 6, 2024 · 2 comments · May be fixed by #630
Labels
high priority Should be prioritised over all other issues question Discussion is required standards Related to coding standards

Comments

@taylortom
Copy link
Collaborator

taylortom commented Feb 6, 2024

We need to establish the best way to define dependencies between Adapt modules*

Adapt modules fall into two categories:

  1. 'Hard' dependencies - where a module needs to use a specific export from another Adapt module (e.g. if extending from another class such as AbstractModule). In this case, a missing module will likely cause an error during app initialisation.
  2. 'Loose' dependencies - where a module imports another module using the AAT's internal dependency manager (i.e. waitForModule). In this case, a missing module will likely cause an unexpected runtime error at some point in the future.

We have the following requirements of Adapt module dependency management:

  • Should be able to specify which modules will be installed for a specific AAT version
  • Should be able to install all Adapt module dependencies automatically with minimal user input
  • Should be able to define a specific (and fixed) set of Adapt modules/versions for reasons of stability
  • Should be able to specify which other Adapt modules an individual module requires in order to run as expected
  • Should be able to customise the list of Adapt modules for individual installations (additional modules as well as overrides)
  • Should be able to override Adapt module code with local code for dev purposes

We currently handle all interdependencies between Adapt modules with NPM, using a mix of NPM's dependencies, peerDependencies, and a package-lock.json file.

On peerDependencies (from the NPM docs):

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin. Notably, your module may be exposing a specific interface, expected and specified by the host documentation.

Whether this is the most sensible approach remains to be seen. This issue has been created for discussion purposes.

See the below issues for some related discussion:

Versioning

Following the above, a solution for versioning a current state of the app/dependencies needs to be agreed upon and documented. We want simple (minimal effort required from devs) and robust (version clamping within a single AAT release).

As with the framework, we should introduce automation via GitHub actions where possible. We need to agree on how releases are triggered (automatic or manual) - It's likely to be a common occurrence that a feature requires changes in multiple modules, so an automatic release mechanism for adapt-authoring may not be possible (although this could/should be implemented for the Adapt modules).

  • How do we handle the CHANGELOG

Use cases

See below for some common example use cases for a dependency management mechanism:

  • TODO

Please comment below with thoughts/opinions.

* Where an 'Adapt module' here refers to the AAT-specific type of Node modules which form the building blocks of the AAT v1.

@taylortom taylortom added question Discussion is required standards Related to coding standards high priority Should be prioritised over all other issues labels Feb 6, 2024
@chris-steele
Copy link

I'll try to explain by way of example my observations/concerns with the way we handle dependencies in the AAT.

adaptframework specifies content as a dependency. If I am working on a dev branch of content in my local workspace then when Node comes to resolve the content dependency for adaptframework it will install a copy of the master branch of content locally in adaptframework/node_modules.

Now, given that Node links workspace folders into the root node_modules folder it will be the dev branch of content that gets loaded via the AAT's internal dependency loader. And given that code requiring content does so via this mechanism then the expected behaviours are obtained. However, if code was imported via ESM then problems would likely occur...

I noticed certain base packages like api/core were specified as dependencies in several packages. If dev work was to be done on these base packages then we'd likely find that packages load master branch code rather than the expected workspace code. Consequently I've amended packages specifying these as dependencies by making the base packages peerDependencies.

This still leaves the problem of AAT packages that specify other AAT packages as dependencies: things may work because dependencies are resolved via the AAT runtime dependency loader, but it is somewhat by fluke and at best it is clumsy to have multiple copies of packages installed and unused.

@chris-steele
Copy link

chris-steele commented Feb 20, 2024

Update following discussion with @taylortom and @oliverfoster:

  • dependency graph to be produced
  • use graph to identify peer and circular dependencies
  • consider integrating adaptframework dependencies into adaptframework
  • look into using Git with Node workspaces, but fall back to Git clones in root node_modules

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 question Discussion is required standards Related to coding standards
Projects
Status: Awaiting Response
Development

Successfully merging a pull request may close this issue.

2 participants