aspnet / LibraryManager

MIT License
458 stars 80 forks source link

Allow specifying destination per file #702

Open Visne opened 2 years ago

Visne commented 2 years ago

Is your feature request related to a problem? Please describe.

I would like to be able to specify where each downloaded file goes in my project.

The README says that one of the reasons to use LibraryManager is "For orchestrating file placement within your project", so this seems to be in the scope of the project.

Describe the solution you'd like

I'm not sure what the cleanest way would be to do this in JSON, but it would be very nice to be able to override for every file or folder a new destination. Currently, you are essentially forced to take over the folder structure of the project you are downloading, which is annoying. For example with signalr, if you are only interested in the files in dist/browser then you are now forced to also have the dist/browser structure in your own project.

Describe alternatives you've considered

Of course you could just move the files manually every time, but this removes all advantages you get from libman clean, libman restore, libman update etc.

ruslan-mogilevskiy commented 1 year ago

@jimmylewis , are there plans to add this feature?

jimmylewis commented 1 year ago

@ruslan-mogilevskiy yes, I'd like to, but I haven't had time. This is a big feature to work out all the details: it has fairly broad impact from changes to the JSON schema, changes to how we pre-validate the manifest (including support for file glob patterns and duplications), and the actual implementation of file placement, so it needs to be clearly spec'ed out. I'd welcome contributions for this, with the caveat of it will likely be fairly involved and my time right now is limited.

Design suggestions would be great too, if there's a way you'd like to see this look in the libman.json file. Having a few ideas or a consensus of what folks would find intuitive and easy to use might help with guiding the code changes.

Visne commented 1 year ago

In my opinion this should work similarly to how the mv command works (in a Bash context). That is, allow moving individual files, or use globbing to move multiple files (without preserving directory structure). Many developers are already very familiar with the syntax for globbing, and it is reasonably powerful.

The config file would then look something like this:

{
    "version": "1.0",
    "defaultProvider": "cdnjs",
    "defaultDestination": "js/lib", // The destination would act similar to a "working directory" for mapping
    "libraries": [
        {
            "library": "someLibrary",
            "mapping": {
                // Basic file placement with renaming
                // Would result in `my/preferred/location/custom_filename.js`
                "cdn/directory/structure/file.js": "my/preferred/location/custom_filename.js",

                // Placing the file in a directory without changing the filename (trailing slash required)
                // Would result in `my/preferred/location/file.js`
                "cdn/directory/structure/file.js": "my/preferred/location/",

                // Placing a full directory
                // Would result in alll files in some/dir/ to be placed in my/preferred/location/
                "some/dir/": "my/preferred/location/",

                // Using globbing
                // Would result in all .js files in some/dir/ ending up in somewhere/
                // Note that while the trailing slash on somewhere/ would not be required, when globbing is
                // used it is always interpreted as a directory, as globbing can match 0 to infinite files
                "some/dir/*.js": "somewhere/",

                // Using directory globbing
                // Would result in all .js files in the some/dir/ and all its subdirectories being moved to somewhere/
                // Note that this could lead to filename collisions, an error should be shown to the user when this 
                // happens telling them what name collision happened and what mapping rule caused it
                "some/dir/**/*.js": "somewhere/"
            }
        }
    ]
}

Some special notes I can think of:

jimmylewis commented 1 year ago

We already use the minimatch library in libman, glob support was added a while back for the existing scenario. The Microsoft.Extensions.FileSystemGlobbing library has some behavioral differences, but mostly is harder to use because it works directly against the filesystem, which we don't have when computing these matches during pre-validation (we compute the globs against the file metadata, not files on disk).

Here's some edge cases building on your example:

{
  "version": "1.0",
  "defaultProvider": "cdnjs",
  "defaultDestination": "js/lib"m
  "libraries": [
    {
      "library": "fileMappedTwiceExample",
      "mapping": {
        // is it valid to install the same file to two separate locations via separate patterns? (seems like it would be allowed, as long as it doesn't create conflicts)
        "files/foo.js": "somewhere/"
        "**/foo.js": "somewhereElse/"
    },
    {
      "library": "fileMappedTwiceExample2",
      "mapping": {
        // What if I want to duplicate a file?  This isn't valid JSON because the property name is reused.
        "files/foo.js": "somewhere/"
        "files/foo.js": "somewhereElse/"
      }
    },
    {
      "library": "fileConflictsExample",
      "mapping": {
        // We need to ensure no collisions across all files + glob expansions
        "a/*.js": "somewhere/"
        // b/ should not contain any files with the same name as a/
        "b/*.js": "somewhere/"
        // there should not be another bar.js in a/ or b/
        "c/foo.js", "somewhere/bar.js" 
      }
    }
  ]
}

Also, preserving directory structure I think is an important, or at least reasonably common, scenario to try to address. Some packages like bootstrap (when fetched via jsdelivr or unpkg) have a top level "dist" folder, and we've seen this feature requested a number of times to try to collapse that. Something maybe like this?

    {
      "library": "bootstrap@latest",
      "mapping": {
        // trying to remove the "dist" folder
        "dist/**/*": "**/*" // special case - if ** exists on both sides, make the result match the source expansion?
      }
    }

If we do support this, do we try to support complex glob patterns with multiple matches (e.g. `/foo/*/.js`)?

Another slightly different approach I was thinking of was to make the mappings be an array of objects. This allows the duplicates in my second example, because they could just be in separate objects, e.g.

{
  "version": "1.0",
  "defaultProvider": "cdnjs",
  "defaultDestination": "js/lib"
  "libraries": [
    {
      "library": "fileMappedTwiceExample2",
      "mappings": [
        {
          "files/foo.js": "somewhere/",
          "styles/*.css": "anotherFolder/"
        },
        {
          "files/foo.js": "somewhereElse/"
          // don't need the same mappings, but this allows duplicating the property name
        }
      ]
    }
  ]
}

Maybe it could be defined as either an array or an object, to enable this special case while keeping it as simple as possible for the majority scenario? Or is that too much of an edge case to worry about supporting?

Visne commented 1 year ago

but mostly is harder to use because it works directly against the filesystem

Yeah, that was a problem I was thinking about but I forgot LibraryManager already uses Minimatch.

is it valid to install the same file to two separate locations via separate patterns? (seems like it would be allowed, as long as it doesn't create conflicts)

Yes, I think all moves should be validated beforehand and if there is a conflict the user should get an error.

What if I want to duplicate a file? This isn't valid JSON because the property name is reused.

That's a good point, I hadn't considered that yet. I think the only good solution to that is to map to an array like so:

"mapping": {
    "files/foo.js": [
        "somewhere/",
        "somewhereElse/"
    ]
}

It will probably be slightly annoying to do in C# though because of static typing (assuming the JSON is deserialized to an object).

trying to remove the "dist" folder

Well, don't forget that the example you gave is already trivial to do by just moving the directory, instead of using glob patterns, like so:

"dist/": "."

Still, this does indeed not allow for much freedom and when multiple ** patterns are used this creates problems. So to anwer your follow-up question:

do we try to support complex glob patterns with multiple ** matches

I think that if we really do want this, we should handle it similarly to RegEx, in that you essentially have capture groups that you can refer to later (although globbing is simple enough that capture groups don't have to be explicitly stated, instead all ** and * can become capture groups automatically). Apparently Apache Struts already does this. Using their syntax, the mapping would look like this:

"**/foo/**/*.js": "{2}/"

This would put all .js files that are in a subdirectory of any foo/ directory, into directories matched by the second ** (while maintaining the structure). I think the main argument against this would be that it adds a ton of complexity, while most projects should be small enough that even mapping all files/directories individually would already be easy enough.

make the mappings be an array of objects

I don't really like this, because it keeps the targets of the file you want to duplicate far away from each other, and it will be confusing to developers that don't need to duplicate files. I think the idea I described before would be better, where you optionally give an array of target locations instead of a single target location.

AraHaan commented 1 year ago

Another pain I face:

{
  "version": "1.0",
  "defaultProvider": "jsdelivr",
  "libraries": [
    {
      "provider": "cdnjs",
      "library": "jquery-validation-unobtrusive@4.0.0",
      "destination": "Scripts/jquery-validation-unobtrusive/"
    },
    {
      "library": "jquery-validation@1.19.5",
      "destination": "Scripts/jquery-validation/",
      "files": [
        "dist/additional-methods.js",
        "dist/additional-methods.min.js",
        "dist/jquery.validate.js",
        "dist/jquery.validate.min.js",
        "dist/localization/messages_ar.js",
        "dist/localization/messages_ar.min.js",
        "dist/localization/messages_az.js",
        "dist/localization/messages_az.min.js",
        "dist/localization/messages_bg.js",
        "dist/localization/messages_bg.min.js",
        "dist/localization/messages_bn_BD.js",
        "dist/localization/messages_bn_BD.min.js",
        "dist/localization/messages_ca.js",
        "dist/localization/messages_ca.min.js",
        "dist/localization/messages_cs.js",
        "dist/localization/messages_cs.min.js",
        "dist/localization/messages_da.js",
        "dist/localization/messages_da.min.js",
        "dist/localization/messages_de.js",
        "dist/localization/messages_de.min.js",
        "dist/localization/messages_el.js",
        "dist/localization/messages_el.min.js",
        "dist/localization/messages_es.js",
        "dist/localization/messages_es.min.js",
        "dist/localization/messages_es_AR.js",
        "dist/localization/messages_es_AR.min.js",
        "dist/localization/messages_es_PE.js",
        "dist/localization/messages_es_PE.min.js",
        "dist/localization/messages_et.js",
        "dist/localization/messages_et.min.js",
        "dist/localization/messages_eu.js",
        "dist/localization/messages_eu.min.js",
        "dist/localization/messages_fa.js",
        "dist/localization/messages_fa.min.js",
        "dist/localization/messages_fi.js",
        "dist/localization/messages_fi.min.js",
        "dist/localization/messages_fr.js",
        "dist/localization/messages_fr.min.js",
        "dist/localization/messages_ge.js",
        "dist/localization/messages_ge.min.js",
        "dist/localization/messages_gl.js",
        "dist/localization/messages_gl.min.js",
        "dist/localization/messages_he.js",
        "dist/localization/messages_he.min.js",
        "dist/localization/messages_hr.js",
        "dist/localization/messages_hr.min.js",
        "dist/localization/messages_hu.js",
        "dist/localization/messages_hu.min.js",
        "dist/localization/messages_hy_AM.js",
        "dist/localization/messages_hy_AM.min.js",
        "dist/localization/messages_id.js",
        "dist/localization/messages_id.min.js",
        "dist/localization/messages_is.js",
        "dist/localization/messages_is.min.js",
        "dist/localization/messages_it.js",
        "dist/localization/messages_it.min.js",
        "dist/localization/messages_ja.js",
        "dist/localization/messages_ja.min.js",
        "dist/localization/messages_ka.js",
        "dist/localization/messages_ka.min.js",
        "dist/localization/messages_kk.js",
        "dist/localization/messages_kk.min.js",
        "dist/localization/messages_ko.js",
        "dist/localization/messages_ko.min.js",
        "dist/localization/messages_lt.js",
        "dist/localization/messages_lt.min.js",
        "dist/localization/messages_lv.js",
        "dist/localization/messages_lv.min.js",
        "dist/localization/messages_mk.js",
        "dist/localization/messages_mk.min.js",
        "dist/localization/messages_my.js",
        "dist/localization/messages_my.min.js",
        "dist/localization/messages_nl.js",
        "dist/localization/messages_nl.min.js",
        "dist/localization/messages_no.js",
        "dist/localization/messages_no.min.js",
        "dist/localization/messages_pl.js",
        "dist/localization/messages_pl.min.js",
        "dist/localization/messages_pt_BR.js",
        "dist/localization/messages_pt_BR.min.js",
        "dist/localization/messages_pt_PT.js",
        "dist/localization/messages_pt_PT.min.js",
        "dist/localization/messages_ro.js",
        "dist/localization/messages_ro.min.js",
        "dist/localization/messages_ru.js",
        "dist/localization/messages_ru.min.js",
        "dist/localization/messages_sd.js",
        "dist/localization/messages_sd.min.js",
        "dist/localization/messages_si.js",
        "dist/localization/messages_si.min.js",
        "dist/localization/messages_sk.js",
        "dist/localization/messages_sk.min.js",
        "dist/localization/messages_sl.js",
        "dist/localization/messages_sl.min.js",
        "dist/localization/messages_sr.js",
        "dist/localization/messages_sr.min.js",
        "dist/localization/messages_sr_lat.js",
        "dist/localization/messages_sr_lat.min.js",
        "dist/localization/messages_sv.js",
        "dist/localization/messages_sv.min.js",
        "dist/localization/messages_th.js",
        "dist/localization/messages_th.min.js",
        "dist/localization/messages_tj.js",
        "dist/localization/messages_tj.min.js",
        "dist/localization/messages_tr.js",
        "dist/localization/messages_tr.min.js",
        "dist/localization/messages_uk.js",
        "dist/localization/messages_uk.min.js",
        "dist/localization/messages_ur.js",
        "dist/localization/messages_ur.min.js",
        "dist/localization/messages_vi.js",
        "dist/localization/messages_vi.min.js",
        "dist/localization/messages_zh.js",
        "dist/localization/messages_zh.min.js",
        "dist/localization/messages_zh_TW.js",
        "dist/localization/messages_zh_TW.min.js",
        "dist/localization/methods_de.js",
        "dist/localization/methods_de.min.js",
        "dist/localization/methods_es_CL.js",
        "dist/localization/methods_es_CL.min.js",
        "dist/localization/methods_fi.js",
        "dist/localization/methods_fi.min.js",
        "dist/localization/methods_it.js",
        "dist/localization/methods_it.min.js",
        "dist/localization/methods_nl.js",
        "dist/localization/methods_nl.min.js",
        "dist/localization/methods_pt.js",
        "dist/localization/methods_pt.min.js"
      ]
    },
    {
      "provider": "cdnjs",
      "library": "jquery@3.6.4",
      "destination": "Scripts/jquery/"
    },
    {
      "provider": "cdnjs",
      "library": "modernizr@2.8.3",
      "destination": "Scripts/modernizr/",
      "files": [
        "modernizr.js"
      ]
    },
    {
      "provider": "cdnjs",
      "library": "bootstrap@5.2.3",
      "destination": "Scripts/bootstrap/",
      "files": [
        "js/bootstrap.bundle.js",
        "js/bootstrap.bundle.js.map",
        "js/bootstrap.bundle.min.js",
        "js/bootstrap.bundle.min.js.map",
        "js/bootstrap.esm.js",
        "js/bootstrap.esm.js.map",
        "js/bootstrap.esm.min.js",
        "js/bootstrap.esm.min.js.map",
        "js/bootstrap.js",
        "js/bootstrap.js.map",
        "js/bootstrap.min.js",
        "js/bootstrap.min.js.map"
      ]
    },
    {
      "provider": "cdnjs",
      "library": "bootstrap@5.2.3",
      "destination": "Content/bootstrap/",
      "files": [
        "css/bootstrap-grid.css",
        "css/bootstrap-grid.css.map",
        "css/bootstrap-grid.min.css",
        "css/bootstrap-grid.min.css.map",
        "css/bootstrap-grid.rtl.css",
        "css/bootstrap-grid.rtl.css.map",
        "css/bootstrap-grid.rtl.min.css",
        "css/bootstrap-grid.rtl.min.css.map",
        "css/bootstrap-reboot.css",
        "css/bootstrap-reboot.css.map",
        "css/bootstrap-reboot.min.css",
        "css/bootstrap-reboot.min.css.map",
        "css/bootstrap-reboot.rtl.css",
        "css/bootstrap-reboot.rtl.css.map",
        "css/bootstrap-reboot.rtl.min.css",
        "css/bootstrap-reboot.rtl.min.css.map",
        "css/bootstrap-utilities.css",
        "css/bootstrap-utilities.css.map",
        "css/bootstrap-utilities.min.css",
        "css/bootstrap-utilities.min.css.map",
        "css/bootstrap-utilities.rtl.css",
        "css/bootstrap-utilities.rtl.css.map",
        "css/bootstrap-utilities.rtl.min.css",
        "css/bootstrap-utilities.rtl.min.css.map",
        "css/bootstrap.css",
        "css/bootstrap.css.map",
        "css/bootstrap.min.css",
        "css/bootstrap.min.css.map",
        "css/bootstrap.rtl.css",
        "css/bootstrap.rtl.css.map",
        "css/bootstrap.rtl.min.css",
        "css/bootstrap.rtl.min.css.map"
      ]
    }
  ]
}

This feature could actually help with this where I want all of bootstraps js files under Scripts/bootstrap, while also wanting all of the css files under Content/bootstrap. Currently it does not handle duplicate package definitions well like this.

lonix1 commented 9 months ago

Related: https://github.com/aspnet/LibraryManager/issues/407