TrySound / postcss-easy-import

PostCSS plugin to inline @import rules content with extra features
MIT License
200 stars 25 forks source link

Dynamic resolve option (module + globbing) #7

Closed toomuchdesign closed 7 years ago

toomuchdesign commented 8 years ago

Set up a dynamic resolve option to dynamically switch between globbing and module imports.

What's new I created a new resolveDynamic resolver which requires is-glob package and both the original resolvers packed with this plugin. When resolveDynamic is enabled, each @import url is tested against is-glob regex and then processed by one of the original resolvers according to is-globresult.

resolveDynamic option Added a new resolveDynamic option (false by default), which enables dynamic resolve only when set to true and when glob option is false.

Option name dynamicResolve or resolveDynamically might be a better name for the new option ;-).

timkelty commented 8 years ago

Eagerly awaiting a resolution to #6. This was functionality postcss-import 7.x had before it removed globbing, so I'm relying on it.

gunzip commented 8 years ago

same here, please merge :)

snuggs commented 7 years ago

@TrySound #ping? 🚢 :shipit: /cc @toomuchdesign @timkelty @gunzip

simonsmith commented 7 years ago

@TrySound kindly made me a collaborator so we can get some outstanding issues resolved. I'll take a look at this PR later on as it seems like a great addition :)

snuggs commented 7 years ago

Nice @simonsmith. Hope the 👊 bump helped. ;-) Love this project and glad to see you aboard! Gotta keep momentum going. 🙏 What left do we have to do to merge this in?

simonsmith commented 7 years ago

I'm currently just working on upgrading some of the dependencies. Moving to latest AVA broke all the tests bizarrely. Once that is sorted I can address the changes here

toomuchdesign commented 7 years ago

Hi @simonsmith, you probably know, but AVA 0.17.0 changes process.cwd() behaviour. Already hit that once! https://github.com/avajs/ava/releases/tag/v0.17.0

simonsmith commented 7 years ago

@toomuchdesign Oh nice, that might be my exact issue as all the tests were failing to resolve modules. I haven't used AVA before so wasn't up to date on changes. Thanks for the tip!

simonsmith commented 7 years ago

I was also wondering about whether this feature should be a default rather than behind an option. We could remove glob as an option and just let the plugin decide what to do based on isGlob.

So something like this in index.js:

opts.resolve = function(id, base, opts) {
    if (isGlob(id)) {
        return resolveGlob(id, base, opts);
    } 
    return resolveModule(id, base, opts);
};

I can't see a reason why someone would pick glob or resolve if it could be done dynamically.

toomuchdesign commented 7 years ago

Probably someone might want to be able to preventively select just glob or module resolve in order to be sure that nothing is imported by chance. But it's just speculation! Thanks for handling the maintainance , @simonsmith.

Cheers!

simonsmith commented 7 years ago

True, although if you're using globs you would probably know what to expect. For example not caring about order of imports.

My thoughts were that this should work out of the box:

@import "suitcss-base";
@import "./components/*.css";

Are you happy to rebase this with master and/or make changes @toomuchdesign? I can understand if not as it's months old. If not I'll pull your branch and make the updates.

toomuchdesign commented 7 years ago

Ok! I'll try to push something before next Monday.

simonsmith commented 7 years ago

Closing in favour of #13

simonsmith commented 7 years ago

Fixed in https://github.com/TrySound/postcss-easy-import/pull/14