Right now, the tota11y.libraries.yml for this module is setup to load the tota11y JS library from /libraries/tota11y/tota11y.min.js. However, if you are using composer.json to manage your dependencies and download the entire tota11y library from Github, it is hard to configure the library to be downloaded and placed in that folder structure.

Sample composer.json

{
  "repositories": [
    {
      "type": "package",
      "package": {
        "name": "local/tota11y",
        "version": "dev-master",
        "type": "drupal-library",
        "dist": {
          "type": "zip",
          "url": "https://github.com/Khan/tota11y/archive/master.zip"
        }
      }
    }
  ]
}

When the tota11y JS library is downloaded based on this composer.json, the minified copy of the tota11y library will be in: /libraries/tota11y/build/tota11y.min.js. So I propose that we change the path to the library in the libraries.yml to match this path.

CommentFileSizeAuthor
#10 2857103-tota11y-use-dist-library-path-10.patch305 bytescodebymikey
#2 change-library-path-2857103-2.patch305 bytesmrkdboyd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrkdboyd created an issue. See original summary.

mrkdboyd’s picture

mrkdboyd CreditAttribution: mrkdboyd commented
FileSize
change-library-path-2857103-2.patch305 bytes

Here is a patch to change the library path

mikebell_’s picture

mikebell_ CreditAttribution: mikebell_ at Convivio commented

I've set aside some time tomorrow (Friday) to review this. Thanks!

mikebell_’s picture

mikebell_ CreditAttribution: mikebell_ at Convivio commented

I like this approach, I think we can take it a step further as well by including the repository and require in the existing composer.json so you only need to run composer require drupal/tota11y.

We need to consider backwards compatibility for existing users (yes all 3 of them ;)).

mrkdboyd’s picture

mrkdboyd CreditAttribution: mrkdboyd commented

Yeah, I agree about adding the library to the composer.json for the module.

I'm not sure how we could maintain backwards compatibility though. If we include these changes as part of an 8.x-2.0 release, rather than a point release for 1.x, I think breaking changes are considered acceptable? And of course, we could document the changes well for anyone updating. Let me know what you think.

And I can create a new patch that includes adding the library to the module's composer.json.

mrkdboyd’s picture

mrkdboyd CreditAttribution: mrkdboyd commented

Upon further review, it may not be possible to use the module's composer.json to require the library. See #2836244: Update documentation for installing via composer and also #2706433: Composer dist problem - with chosen composer.json (false github auth problem), particularly comment #29.

The best approach may just be to document how to add the dependency for the tota11y library to your project's root composer.json, as in this patch: https://www.drupal.org/files/issues/chosen-fix_composer_installation-283...

  • mikebell_ committed 48d3ef1 on 8.x-2.x authored by mrkdboyd 
    Issue #2857103 by mrkdboyd, mikebell_: Change library path for tota11y
mikebell_’s picture

mikebell_ CreditAttribution: mikebell_ at Convivio commented

Really sorry for being so slow on this.

I've pushed a new branch with these changes in 8.x-2.x

I'll try and test it out fully before making a new release.

B-Prod’s picture

B-Prod CreditAttribution: B-Prod commented
Status: Active » Reviewed & tested by the community

This works file and fix error message when installed using Composer.

codebymikey’s picture

codebymikey CreditAttribution: codebymikey at Zodiac Media commented
Status: Reviewed & tested by the community » Needs review
FileSize
2857103-tota11y-use-dist-library-path-10.patch305 bytes

The url path should now be /libraries/tota11y/dist/tota11y.min.js for the latest version on npm https://registry.npmjs.org/@khanacademy/tota11y/-/tota11y-0.2.0.tgz

ikit-claw’s picture

ikit-claw CreditAttribution: ikit-claw as a volunteer commented
Version: 8.x-1.1 » 2.0.x-dev
Status: Needs review » Fixed

Merged into 2.0.x-dev If you would like to test it that would be great. https://www.drupal.org/project/tota11y/releases/2.0.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.