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

Allow specifying destination per file #702

Open
Visne opened this issue Sep 28, 2022 · 7 comments
Open

Allow specifying destination per file #702

Visne opened this issue Sep 28, 2022 · 7 comments

Comments

@Visne
Copy link

Visne commented Sep 28, 2022 •
edited

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
Copy link

@jimmylewis , are there plans to add this feature?

@jimmylewis
Copy link
Contributor

@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
Copy link
Author

Visne commented Jan 14, 2023 •
edited

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:

  • Since backwards compatibilty would be very important for this (otherwise projects could suddenly break after an update) it is probably a good idea to introduce this as an experimental feature first, in case anything turns out to need drastic changes.
  • Microsoft already has a globbing library which could probably be used to do a ton of the work.
  • One thing that is hard to do with this globbing style is taking all files of a specific type (*.js for example) from a specific directory while keeping the directory structure. Since some/dir/**/*.js would move all .js files to one directory, you would have to use some/dir/*.js and some/other/dir/*.js etc to map all directories individually. I would argue that if you want this functionality but there are too many subdirectories to do this for each individually, then LibraryManager is probably not the tool for you anyways.
  • It is somewhat ambiguous what would happen if "mapping" and packages.files are both specified. Perhaps they should just be mutually exclusive, with the user getting an error when both are specified.

@jimmylewis
Copy link
Contributor

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

Visne commented Jan 14, 2023

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
Copy link

AraHaan commented Apr 9, 2023

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
Copy link

lonix1 commented Dec 23, 2023

Related: #407

@lonix1 lonix1 mentioned this issue Dec 23, 2023
Closed
@jimmylewis jimmylewis mentioned this issue Apr 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jimmylewis @AraHaan @Visne @lonix1 @ruslan-mogilevskiy