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

Add more sensitive plugins tests #157

Merged
merged 3 commits into from
Jan 27, 2016
Merged

Add more sensitive plugins tests #157

merged 3 commits into from
Jan 27, 2016

Conversation

TrySound
Copy link
Member

There are some problems with plugins.

  • They do not process root file (it was in before dev branch).
  • New arch manipulate chunks and to process ast by plugins need to rebuild new ast, apply plugins and then parse in chunks again.
  • Ast is processed by plugins many times depends on how deep imports. It becomes bigger performace problem when we parse chuncks every time.

I think the best decision is remove plugins option.

@TrySound
Copy link
Member Author

Or use it to process source ast like transform but for ast. By the way it's better than stylelint with transform :)

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

Removing is a bad idea for many reason. For example they are useful to process files with stylelint and ensure rules are processed in the original context (eg: postcss-bem-linter needs to be executed on file).
If there is an issue, we need to fix it. People using "plugins" option will not look for the ultimate performance.

@TrySound
Copy link
Member Author

Stylelint with current realisation is unuseful. It won't process root file and will incorrectly process imported with imports. And new realisation will break performance with null plugins too.

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

So how people might use some plugins on imported files now? Like postcss-bem-linter?

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

Perfs is something, but this is a development tool, so we don't need to have the fastest thing possible. We need to think to DX.

@TrySound
Copy link
Member Author

@MoOx I can suggest to process source files after parse before parseStyles().

@TrySound
Copy link
Member Author

And linters will process original files.

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

How importing a file make it "non originial"?!

@TrySound
Copy link
Member Author

How prefixes can make it "non original"?

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

I am not following you.

The original idea of the "plugins" option is to be able to run a plugin on a file when importing it (so when no plugins have run on it yet - so content (ast) as not been modified yet (so it's the "original" source)).

@TrySound
Copy link
Member Author

@MoOx I can follow this idea. But in your case one plugin will be applyed: postcss-import. btw stylelint will be applied multiple times on the same code (if it imported).

@MoOx
Copy link
Contributor

MoOx commented Jan 26, 2016

The way code was was supposed to run to provided "plugins" on imported ast beforing adding them in the main ast.

@TrySound
Copy link
Member Author

.@import 'module1';
lint-warning1 {}
@import 'module2';
lint-warning2 {}
lint-warning3 {}

First step

lint-warning3 {}

Second step

lint-warning3 {}
lint-warning2 {}

Third step

lint-warning3 {}
lint-warning2 {}
lint-warning1 {}

Lint result:

lint-warning3
lint-warning3
lint-warning2
lint-warning3
lint-warning2
lint-warning1

Do you think it's correct?

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Can you explain me this a little more?
plugins might not be just for linting.

@TrySound
Copy link
Member Author

Imported ast is processed by plugins again and again depends on how it deep. And linters with this way can be very dirty.

@TrySound
Copy link
Member Author

It's recursion.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

The order of applying plugins is not important.
What is important is that a imported file must be parsed and processed individually right after it has been read from the fs, to have apply plugins on a fresh ast.

@TrySound
Copy link
Member Author

Yes! I want this. I'm talking about current way when we have a lot of duplicated messages.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Duplicated messages (so processing of plugins, that's what your are talking about?) should not happen.

@TrySound
Copy link
Member Author

I'll do that. But this won't let to use imported variables. BTW with current way we are not able too, caz they definitions are removed before adding to ast.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Using imported variables is working atm. This is a requirement.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Lot's of plugins like postcss-custom-properties, postcss-custom-media etc, rely on that.

@TrySound
Copy link
Member Author

But in this case linters we have not original ast and will warn duplicated messages.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Oh I said something stupid. Those plugins rely on the fact that import will inline (so they use references themselves).

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

So yeah you are right, if someone us a plugin for variable or something like that in "plugins" option, they won't be available for the rest of the ast, sure.

@TrySound
Copy link
Member Author

Root ast still is not processed with plugins. We you want this I'll try to add them in parseStyles.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Root ast should not be processed by plugins in "plugins" option.

@TrySound
Copy link
Member Author

In this case for example lint plugins should be added twice.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

One for the main ast BEFORE postcss-import and one in "plugins" option, sure.

@TrySound
Copy link
Member Author

Okay :) Is it okay now?

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Let's recap: you just fixed/improved the plugins option (that's is supposed to execute plugins on imported ast before they are inline)?

@TrySound
Copy link
Member Author

@MoOx Not exactly. This imported files contain not processed imports.

@TrySound
Copy link
Member Author

For now we just apply plugins on just parsed ast.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Sorry it's so long, it's just to avoid confusion for users. If I am confused, I can't imagine users.

So if some "plugins" make some changes to the ast, it won't be refected in the inlined files?

@TrySound
Copy link
Member Author

All imports and media will be detected after these plugins, so some of them can be removed and won't be imported.

@TrySound
Copy link
Member Author

In previous bahaviour it firstly inline all imports in this file and then apply plugins.

@TrySound
Copy link
Member Author

And plugins processed the same ast parts multiple times.

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

Previous behavior was: read file, create ast, apply "plugins", insert modified ast in the main ast.

@TrySound
Copy link
Member Author

Nope
read file, create ast, insert imported ast, apply "plugins", insert modified ast in the parent ast

@TrySound
Copy link
Member Author

For now
read file, create ast, apply "plugins", insert imported ast, insert modified ast in the parent ast

@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

If the behavior is now "read file, create ast, apply "plugins", insert imported ast, insert modified ast in the parent ast", I am ok with it, since that the one which make sense.
Can you add this in the CHANGELOG?

MoOx added a commit that referenced this pull request Jan 27, 2016
Add more sensitive plugins tests
@MoOx MoOx merged commit 0137426 into dev Jan 27, 2016
@MoOx MoOx deleted the plugins branch January 27, 2016 09:05
@MoOx
Copy link
Contributor

MoOx commented Jan 27, 2016

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

Successfully merging this pull request may close these issues.

2 participants