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

Added autoprefixer to stylus #7727

Merged
merged 3 commits into from Dec 6, 2016

Conversation

Projects
None yet
4 participants
@mitar
Collaborator

mitar commented Aug 29, 2016

Nib is very outdated. I was mostly using it to add vendor prefixes. But now people use autoprefixer for that. So I added it to the package.

One can configure which browsers to support using file options:

  api.addFiles([
    'file.styl'
  ], 'client', {
    autoprefixer: {
      browsers: ['last 3 Chrome versions', 'Firefox >= 7', 'Explorer >= 8', 'last 2 versions', '> 1%', 'Firefox ESR', 'Safari >= 4']
    }
  });

See defaults here.

mitar added a commit to peer/mind that referenced this pull request Aug 29, 2016

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Aug 30, 2016

Hmm, the downside of this approach is you can't configure imported files from the app. I would suspect a way of configuring autoprefixer globally is what we want, but of course this isn't easy to do (package level configuration, especially for a build plugin).

@tmeasday tmeasday self-assigned this Aug 30, 2016

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 30, 2016

Ahh. I completely forgot about app-level content. I am not using that in my apps.

Yea, then we have to do something similar to what Babel has which is ugly. And I was so happy for options to work so well.

Maybe we should simply create a way to configure top-level options for build tools? .meteor/stylus.json would be loaded as options for all .styl files? And each package could have .meteor/<packagename>.json which would add configuration to options for all top-level addFiles?

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Aug 31, 2016

I was thinking more along the lines of figuring out a good way to provide configuration for a build plugin. Maybe a principled version of what juliancwirko:postcss does, which is reading configuration out of package.json.

I know this is something people have wanted for a long time (static package-level config). I'm not sure what the argument is why we've never done it.

@mitar

This comment has been minimized.

Collaborator

mitar commented Aug 31, 2016

Now that we have package.json this might be good place as well.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 6, 2016

Shall we close this and open a FR that is contingent on figuring out global configuration?

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 6, 2016

Wouldn't it be better just to add global configuration code here? When somebody suggests how to do it? I can also give the person access to my branch to do so.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 6, 2016

Maybe you're right. Figuring out a system of global configuration is probably a bit OTT

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 27, 2016

I updated the pull request. Can we get this merged in? ;-)

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 27, 2016

I opened #7829 for global configuration of build plugins.

This was referenced Sep 27, 2016

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Sep 27, 2016

The downside of this approach is that it needs to be done for each and every plugin that wants autoprefixing (sass, less, ...) whereas putting it in the minification step works for every preprocessor.
However, if I'm not mistaking, the minification step doesn't run on css imported through javascript, so this would be a way to still get autoprefixing and import your css in your JS files.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 27, 2016

Also, I need prefixing during development so that I can test things across browsers.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 27, 2016

@mitar what does this do by default? (For files with fileOptions = {}?)

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 27, 2016

so this would be a way to still get autoprefixing and import your css in your JS files.

@sebakerckhof why do you think this code would run over CSS imported from JS?

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 27, 2016

@mitar what does this do by default? (For files with fileOptions = {}?)

There is a link to that in the description of the pull request. ;-)

By default it will automatically prefix anything which has to be prefixed for the following browsers: > 1%, last 2 versions, Firefox ESR.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 28, 2016

This is a fairly large change for the default behaviour. Do you think that may cause problems for people?

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 28, 2016

Doesn't prefixing just fix things, not break things? Can it break anything?

But I can also make it so that it is disabled for now by default and you have to enable it. And then once #94 is resolved we can change that.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 28, 2016

Yeah, do you think I'm being too conservative? It's hard to know if it'll be a problem for people beyond larger file sizes.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 28, 2016

Fine with me. I am using packages-based architecture anyway for my apps. :-)

I think we can be conservative now and work on options. But I would not like that options are blocking this.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Sep 28, 2016

Ok, let me know and I'll merge it.

On Wed, 28 Sep 2016 at 14:31 Mitar notifications@github.com wrote:

Fine with me. I am using packages-based architecture anyway for my apps.
:-)

I think we can be conservative now and work on options. But I would not
like that options are blocking this.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#7727 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIFypPJiWV0gYloxls4e5uZ7uovWYh2ks5que2VgaJpZM4JvOCP
.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 28, 2016

BTW, it seems it also uses environment variables to configure things. And browserslist config file. It would be interesting to see where does it search for.

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Sep 28, 2016

@sebakerckhof why do you think this code would run over CSS imported from JS?

Well, not imported css, but at least imported stylus files. If you'd do the same thing with a preprocessor that accepts normal css syntax (like sass or less) you could just rename your css files to scss or less to get them autoprefixed, even when importing through JS. Or I guess one could also create a build plugin just for autoprefixing normal css files. It's not perfect, but it's probably the only way with the current build system.

Apparently the original plan was for slava to also add a post-compile step in the build tool specially for this kind of use case, but it didn't make it due to time constraints. Even better would be if we could chain build plugins so one could go from scss -> postcss -> whatever you want, but that would add even more complexitiy to the build tool and more configuration for the user.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Oct 4, 2016

@mitar let me know when you've changed the default.

@mitar

This comment has been minimized.

Collaborator

mitar commented Oct 4, 2016

Yes, busy with some other stuff at the moment.

@mitar mitar force-pushed the peerlibrary:stylus-autoprefixer branch from 09e225c to d85be5c Nov 22, 2016

@mitar

This comment has been minimized.

Collaborator

mitar commented Nov 22, 2016

@tmeasday: I changed it so that by default autoprefixer is disabled.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Nov 29, 2016

Can we not run autoprefixer at all if the option is undefined?

@mitar

This comment has been minimized.

Collaborator

mitar commented Nov 29, 2016

That is an option as well. :-)

@mitar

This comment has been minimized.

Collaborator

mitar commented Nov 30, 2016

Updated.

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Dec 6, 2016

Thanks for bearing with me here @mitar

tmeasday added a commit that referenced this pull request Dec 6, 2016

@tmeasday tmeasday merged commit 3c94e69 into meteor:devel Dec 6, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mitar mitar deleted the peerlibrary:stylus-autoprefixer branch Dec 6, 2016

@mjmasn

This comment has been minimized.

Contributor

mjmasn commented Jun 20, 2017

@mitar so for app-level stylus files is there a way to enable autoprefixer? Had to move from mquandalle:stylus to stylus for Meteor 1.6 but just realising prefixing is now broken.

EDIT: Ah, found a solution in the docs eventually. In case anyone else is wondering...

meteor remove standard-minifier-css
meteor add juliancwirko:postcss
meteor npm i -S --save-dev autoprefixer

Also added to package.json

{
    ...
    "postcss": {
        "plugins": {
            "autoprefixer": {
                "browsers": [
                    "last 3 versions"
                ]
            }
        }
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment