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

"main" vs "files" vs "ignore" #69

Open
eurosat7 opened this issue Jul 28, 2016 · 25 comments
Open

"main" vs "files" vs "ignore" #69

eurosat7 opened this issue Jul 28, 2016 · 25 comments

Comments

@eurosat7
Copy link

HI!

Due to

bower/spec#43
and
bower/spec#47 #Proposal 1: files flat list

I would like to use "files" and/or "ignore" in addition to "main" in the bower.json of required packages to find my assets.

Just an example:

    var bowerfiles = require('bower-files')({
        'include':['main','files'],
        'exclude':['ignore']
    });

And/Or:

    bowerfiles.include("main").include("files")
    bowerfiles.all().exclude("ignore").exclude("main").ext("js").files

What do you think?

@ksmithut
Copy link
Owner

I'm still trying to do an in depth read of it, but is it like the 'files' in package.json? If so, the files array is just a list of files that need to be published in the tarbal that gets stored in the npm registry, as with the ignore option. AFAIK, bower links to github and doesn't actually manage the storage of the versioned packages, but I could be wrong. In that case, maybe files and ignore have special meaning that has no relation to how package.json works.

By the looks of some of these issues, I might be able to dramatically reduce the amount of code by leveraging more of the core bower package. It's been a while since I've looked at any of their new stuff. It shouldn't be hard to support this if the bower list command supports them.

@eurosat7
Copy link
Author

That was a quick reply. ;)

Here some more indepth:

We use svn.
So far we were happy using composer and a satis with private repositories for your php needs.

Now we want to manage our own assets/skins/themes in separate packages. And also use external packages like jquery, modernizr and many more via bower.

We have repositories managed via "private bower" as our private "packagist" replacement.

We do not really "publish" versions in tar balls. Instead we work on dev-branches and include them where needed in different projects still in developement. Some are even linked as svn externals.

So now we try to find a way to get out theme-packages and its dependencies compiled (less) and combined and all images inside the css files into the "right" place.

bower no longer likes globbing in "main".

@ksmithut
Copy link
Owner

makes sense. Taking a look at the bower list command, it should be pretty easy to support with with some refactoring. I'll work this weekend to add those as features.

@eurosat7
Copy link
Author

I owe you a pizza. :)

Side note:
Here is a way we could "solve" the problem but that feels totally off:

var globmain = [
    "**js/**.js",
    "**css/**.css",
    "**other/**.assets",
    "**img/**.{png|gif|jpg|svg}"
];
var bowerfiles = require('bower-files')({
    "overrides": {
        "our-package-1": {
            "main": globmain
        },
        [...],
        "our-package-719": {
            "main": globmain
        }
    }
});

@ksmithut
Copy link
Owner

ya, including "files" as an option would feel much cleaner. too many stars :)

@ksmithut
Copy link
Owner

ksmithut commented Aug 1, 2016

@eurosat7 Just an update, still looking into a clean solution. The bower list command is async, which would dramatically reduce the usability of this module. I'm going to dig into the internals of that command and see if I can replicate it with a sync version.

@eurosat7
Copy link
Author

eurosat7 commented Aug 1, 2016

Here was what I am up to [link deleted].

side note: I am also trying to follow composer require definitions as we run a mixed setup.

@ksmithut
Copy link
Owner

ksmithut commented Aug 1, 2016

Sweet :) If you can get that in a PR that would work with this version, that'd be great :) It's been a while since I've touched this code and my javascript style and libraries of choice have changed a lot. I'm considering a refactor with breaking changes, but I'd like to get this feature in before that.

@eurosat7
Copy link
Author

eurosat7 commented Aug 1, 2016

I am not allowed to code for PR at work.

But please(!) : Feel free to rewrite and/or comment/document.
And the less dependencies you have the better. ;-)
And the easier the code is to follow the better. Some parts of the code felt uneasy.

@ksmithut
Copy link
Owner

ksmithut commented Aug 1, 2016

Ya, there's a lot I don't like about they way I did this. I'm going to take a more functional approach in the future.

Oh man, that's a rough work rule. Working for a company that leverages open source but isn't willing to contribute back always feels like bad mojo.

I'll work on the refactor and try to get this in as soon as possible. Sorry for the delay!

@eurosat7
Copy link
Author

eurosat7 commented Aug 2, 2016

Feel free to contact me via mail.

@ksmithut
Copy link
Owner

ksmithut commented Aug 9, 2016

@eurosat7 Have you tried using wiredep? I'm beginning to think they're way ahead of the game as far as dependency resolving goes. Pretty clean codebase, and lots more updated than mine. They seem to be more spec compliant than me, and they seem to always be on the radar with the bower team.

@eurosat7
Copy link
Author

I might try wiredep or webpack in the next project. I looked at them some weeks ago and like their approaches. But for now legacy stuff is hindering me and I had to create an individual solution.

Feel free to reject and close this issue if you think it's not part of this package after having a closer look.
Thanks for your time. :)

@ksmithut
Copy link
Owner

No, I still plan on adding this feature. It's just that the more I read about this "files" thing, the more I'm saddened that I didn't keep up. It sounds like they kind of stepped away from the "main" property as the canonical source of having a list of files. Although, even if I had kept up, I doubt that all bower packages would have updated accordingly. I'll work on this today

@ksmithut
Copy link
Owner

@eurosat7 I have a pull request in that I'm planning on modifying based on your feedback.

#70

Basically, I added an option .useFiles() that will opt to use the files property instead of the main property. If the "files" property isn't present, it will use the "main" property. I still don't feel super good about it, but if this would work for your case, I'll merge it in a publish a new version. I really don't like the way I've organized my code and I'd like to rewrite it using a more functional approach and is more in line with the bower spec. Let me know what you think of what the behavior should be (with fallbacks and such).

@eurosat7
Copy link
Author

NICE :)

Automatic switching between "main" and "files"? Should be optional.

I would have done it a little more generic: .setFilelistProperties(["main","files"])

So one could add custom properties for his own build tools or when the specifications change again. (default should stay ["main"])

But I would do it another way:

  • You can define if all properties inside the setFilelistProperties() should be read or if the reading should stop when the first property with data was found. .readAllFilelistProperties(true) (default being true?)
  • And the order in which the properties are checked is the same than it was defined via setFilelistProperties().

Anyway: Don't foget to have a test where you check for glob-support, which got forbidden lately for "main" but is still valid for "files": {"files" : ["css/**.css"]}

PS:
An idea without comment:
.setRequireProperties(["require","require-dev","dependencies","devDependencies"]) and .readAllRequireProperties(true)
.setJsonFiles([".bower.json","bower.json","component.json","composer.json","package.json"]) and .readAllJsonFiles(true)

stay cool, don't close the fridge.

@ksmithut
Copy link
Owner

I like it! thanks for your suggestions.

How about this?

// This gets all files from the files property, then falls back to the 'main' property if the 'files'
// property doesn't exist, in that order
lib.fileListProps(['files', 'main']).files

// This gets all files from the files property, and the main property
lib.fileListProps(['files', 'main'], false).files

With both of those, glob matching works, and it still only pulls out unique files, so if there is overlap between main and files, it only gets the unique files.

@eurosat7
Copy link
Author

Your comment on the second one is incomplete. But that aside: I'm happy with that. :)

About the "ignore" - Properties: Are they applied as a filter by default or do you only apply them if the user does some .fileListIgnoreProp(false) default being .fileListIgnoreProp(['ignore'])?

@ksmithut
Copy link
Owner

Hmmm... I guess that's a good point about the ignore properties. Just so I'm clear, ignore properties should be applied after files, right? The "files" property can have glob matching, which is the reason you would want ignore? I guess another question is, if one of the file entries is a directory, should all of the files contained in that directory be included?

Oh yes, and the other options you mentioned: I guess there may be a use case where you might want the contents of the other json files read, but for the most part you probably would only need that for a package or two that didn't fill out their bower.json correctly, and you wouldn't want it applied globally to all bower components. That's why there is an overrides option so that you can override on a package-by-package basis. If, however, there is another package like this one or wiredep or main-bower-files that support those types of fallback options for require properties and json files, I'll do what they do. I'll take a gander around npm to see what I can find. Is there a specific bower component that you're having issue with their bower.json setup?

@eurosat7
Copy link
Author

eurosat7 commented Aug 16, 2016

  1. question:
    Yes: ignore is "stronger". (Doing it in another way would make using ignore unpredictable).
  2. question
    They should be forced to glob. So I'd say no to matching whole directories by default - but having an option to change that behaviour might be polite. The specifications are focussed on selecting files.

2nd paragraph 1. topic
Some old packages do not have the bower.json but the good ol' component.json (jquery 1.5.2 for example)

2nd paragraph 2. topic
You can use fxp/composer-asset-plugin (npm) to make composer work with bower-packages so you can checkout composer packages AND bower packages into the same folder with the composer tool.

If you then want to also work on composer packages you will not have a bower.json but a composer.json.

So I thought it would be nice to widen up the scope of the tool.

@ksmithut
Copy link
Owner

@eurosat7 Sorry it's been a while. I'll address the ignore piece today, and stick to globbing.

As far as supporting multiple package formats, I think it would be difficult to accommodate all the different packages that don't comply with bower/component standards. Ultimately, accommodating multiple standards results in code bloat (of which bower-files is already victim of due to my poor design choices) and code poor maintainability, meaning I might be stuck using old standards for a while, which I guess I'm doing right now anyway... But for now, you can use the overrides options to fix up those pesky packages.

I think I'm going to tackle the files/ignore properties for this PR, and attempt to catch this library up before adding more customization options.

@ksmithut
Copy link
Owner

Alright, I've updated the pr to include a .ignoreListProps() method. If this is sufficient for now, I'll merge the PR and publish a new version. I'll likely keep this issue open to to track our conversations on the multiple package formats and such.

@eurosat7
Copy link
Author

eurosat7 commented Aug 30, 2016

The default for ignoreListProps() being [] is ok, as long as "files" is not in the default for fileListProps().
So I think it's ok.

I'll stay subscribed if you want to exchange ideas. Stay happy. ;)

@ksmithut
Copy link
Owner

ksmithut commented Sep 6, 2016

Yes, the "files" is not the default for this version, since that would be a breaking change. Using either of the new methods are opt-in. In the next major version of bower-files, the default will be spec compliant. I've got another bug to work in, but then I'll publish a new version this week.

@ksmithut
Copy link
Owner

ksmithut commented Sep 8, 2016

@eurosat7 After all this time, I've finally pushed out the change to 3.14.1. Let me know if you have any issues with these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants