search

OverZealous / cdnizer

Node module for replacing local links with CDN links, includes fallbacks and customization
MIT License
52 stars 24 forks source link

Look for package version in NPM files #28

Closed olibri-us closed 6 years ago

olibri-us commented 6 years ago

Hi, I added the option to look for version in files "package.json" too since it was locked to "bower.json" and I don't use bower ;)

If you have some time, could you release a new version including it directly in NPM please ? Would be a real good thing for me :)

Anyway, thanks for you attention

OverZealous commented 6 years ago

This looks great! I'm shocked it was so simple.

I'm going to merge this even though it's "failing" (it's simply due to being an old project where I've never locked down the NPM versions, and still testing on very old versions of node). I'll clean up the repo a touch, and get it deployed.

I'm pretty sure this should be safe, so I'll probably just release it as a minor version bump. Should be out pretty quickly.

olibri-us commented 6 years ago

Well it could've been a little bit more "configurable" if I added an option to tell the filename in the config, but I ain't got much time for this now :) I've been using this fix for a while now, it never broke anything for me ;) I'm also using your other gulp modules and lovin them :+1:

Do you think you'd have time to release some updated NPM version someday ? Have a great day

OverZealous commented 6 years ago

Great! I'll add some tests and some docs around it, and maybe tweak the option name so it's a little more usable.

olibri-us commented 6 years ago

Ok I might do it too if I have some time soon but right now work is hectic :/

OverZealous commented 6 years ago

I understand, I should have it up today, so no worries.

OverZealous commented 6 years ago

Hope it's not an issue, but since I think node_modules should be the default functionality (Bower is pretty much dead now), I'm going to have to release this as a major change (it's pretty breaking otherwise).

So, by default, cdnizer will now look for a matching file in this pattern:

And you'll be able to override both node_modules and bower_components using bowerComponents and nodeModules.

If you don't have any overlapping packages, then there wouldn't be a configuration change. However, if someone had the same package in both node_modules and bower_components, but with different versions, they could get unexpected behavior, which is why this becomes a breaking change.

Still, I think it's worth it. I might change some config defaults at the same time.

olibri-us commented 6 years ago

Well I do agree that Bower is on the decline, but still some people use it. Your idea seems very good to me, and it looks non breaking. In the case you do have both packages, i'd suggest that it would fall back to NPM maybe ?

If you want to make it really backward compatible, you just need to deprecate the "bowerComponents" option and add a new one that would override it (maybe "packageFolder" that could be either bower_components or node_modules) and also a packageVersionFilename (or something like this, which could be bower.json or package.json and has to include a "version" key containing version number)

Actually it would be more accurate considering current context to default to NPM but would break existing configs (unless ignored if bowerComponent is defined ?)

olibri-us commented 6 years ago

Still it's a very nice tool, made me save plenty of time to integrate CDNs in my app ! I think it should regain some popularity when people realize it can be use with node packages...

OverZealous commented 6 years ago

Yeah, it's going to default to looking into node_modules first. If that file exists (with package.json), it'll just use that. Otherwise, it'll try bower_component. So, like you said, it's only breaking if the same package was installed with both Bower and NPM, and don't have the same version.

It's still likely to cause an issue for people, hence releasing this as 2.0.0. But the changes I'm making should let you strip out some configuration, since any packages under node_modules will automatically be detected.

I'm glad you like it! I don't actually use it in my own work, because we can't use traditional CDNs for certain reasons, but I'm happy people can still find a use for it!

olibri-us commented 6 years ago

Ok suits me very well ! I do use some of your packages, like run-sequence and lazypipe (and the gulp adaptor for cdnizer) so I'm glad I could help improve one of them !

OverZealous commented 6 years ago

OK, it's been deployed as version 2.0.0 both for cdnizer and gulp-cdnizer!

olibri-us commented 6 years ago

Wow so fast thanks a lot !