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

File containing same content shouldn't be skipped #67

Closed
hgl opened this issue Jul 9, 2015 · 15 comments
Closed

File containing same content shouldn't be skipped #67

hgl opened this issue Jul 9, 2015 · 15 comments

Comments

@hgl
Copy link

hgl commented Jul 9, 2015

Currently, if a file contains the same content as another, it's skipped. This can fail in cases like this:

/* main.css */
@import "foo/index.css";
@import "bar/index.css";
/* both foo/index.css and bar/index.css has this content  */
@import "baz/index.css";

This causes bar/baz/index.css not being imported.

@hgl
Copy link
Author

hgl commented Jul 9, 2015

I'm not sure I understand the rationale behind hashing a file, so not sure how to send a PR.

Shouldn't files be skipped only according to their absolute paths? Maybe I missed something?

@MoOx
Copy link
Contributor

MoOx commented Jul 9, 2015

"Proxy" files (which contain only some @import) should be imported all the times, never kipped.

@MoOx
Copy link
Contributor

MoOx commented Jul 9, 2015

But if full paths are exactly the same, the file will be imported once (test should be made on absolute path)

@hgl
Copy link
Author

hgl commented Jul 16, 2015

Well, since the title fits what I'm about to say. I will just post here.

Imagine this css set up:

@import "tabs/base/index.css";
@import "tabs/pretty/index.css";

tabs/base/index.css:

@import "layout.css";
@import "color.css";
@import "background.css";

tabs/pretty/index.css:

@import "color.css";
@import "background.css";
@import "typography.css";

Both tabs/base/background.css and tabs/pretty/background.css:

.tabs-tab {
    background: url(tab-bg.png)
}

.tabs-panel {
    background: url(panel-bg.png)
}

In this case, the base module provides basic styles, and the pretty module provides more vibrant styles.

If you skip files based on content, backgrounds offered by the pretty module are ignored.

@hgl
Copy link
Author

hgl commented Jul 16, 2015

Probably not skipping files with same absolute url is too much, and multiple modules importing the same normalize.css file is understandable, but files with different absolute urls and same content should not be skipped, i believe.

@MoOx
Copy link
Contributor

MoOx commented Jul 16, 2015

In order to prevent use case like you mentionned, I was thinking we could (in addition to the content, add a minimal length (in characters) where we can consider files are likely to be the same. But since this is starting to be ridiculous, I guess we should just add an option to enable/disable skipping like it is atm. PR is welcome for a skipDuplicates option (like I said in the other PR, true by default to release this as a minor change for now).

Reminder: this "feature" is out for months and you are the first to complain about it. I am going to add some documentation about that in order to make it clear for everyone. Your PR (if you are going to do it) might helps others like you. If some others people come to yell, we might change default behavior in a major release.

@hgl
Copy link
Author

hgl commented Jul 16, 2015

I wonder what's the drawbacks of not skipping files with different absolute urls but same content? The normalize.css case you mentioned certainly don't fail under it.

I don't think cssnext has gained big enough user base (I just compare the download number on npm to that of coffee-script, hope you don't mind) to have users to complain as soon as something might not work correctly. So I don't think rely on feedback is a good way for cssnext to proceed right now, especially the use case is a rare one like I mentioned (I personally haven't been hit by that yet, just come up with it after read the source of this plugin).

@MoOx
Copy link
Contributor

MoOx commented Jul 16, 2015

Tip: if you want to go forward quickly, start with small steps ;)

I personally haven't been hit by that yet, just come up with it after read the source of this plugin

That's why I mentionned that I will update the doc to make the current behavior clear for everyone.

@hgl
Copy link
Author

hgl commented Jul 16, 2015

Sure, sounds good to me.

I think i should wait to complain about skipping once I really encounter one in my project. :)

@hgl hgl closed this as completed Jul 16, 2015
@MoOx
Copy link
Contributor

MoOx commented Jul 16, 2015

I will let this issue opened since I think we should do something (at least option to modify + doc).

@opeologist
Copy link

i'm having this issue as well. trying to convert over from sass to postcss and the framework i'm using templates like the original post. please let me know when this PR is done, or maybe point me in the direction of where this switch would be implemented, as i'd rather get this option sooner rather than later :) thanks!

@opeologist
Copy link

nvm i see what's happening. i'll fork and work on it until either someone else PRs or i finish

@MoOx MoOx closed this as completed in 2466a53 Jul 21, 2015
@hgl
Copy link
Author

hgl commented Jul 21, 2015

Thanks for the options.

Although I think it covers two different kinds of skips:

  1. Skip importing the same file more than once.
  2. Skip importing different files with same content.

There are probably difference use cases in both kinds. I personally want to enable #1 but disable #2, but it doesn't seems to be achievable with the current single option design.

@magsout
Copy link
Member

magsout commented Jul 21, 2015

👌good job @MoOx

@opeologist
Copy link

gonna check out the commit in the morning. thank you so much!! :D

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

4 participants