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 test for nested import ordering #169

Merged
merged 1 commit into from
Nov 2, 2016

Conversation

adam-h
Copy link
Contributor

@adam-h adam-h commented Feb 8, 2016

From #168

@MoOx
Copy link
Contributor

MoOx commented Feb 8, 2016

@TrySound do you plan to have a look to this (since you introduced the responsibles changes)?

@TrySound
Copy link
Member

TrySound commented Feb 8, 2016

Sure. Will look tomorrow.

@TrySound
Copy link
Member

I understand the problem. It's all about skipping duplicates. If we want pure duplcations resolving we should do it after building tree. But it's very massive operation. But if current behaviour creates issues should we implement?

@MoOx
Copy link
Contributor

MoOx commented Feb 14, 2016

Probably

@adam-h
Copy link
Contributor Author

adam-h commented Feb 15, 2016

I'd like to advocate for 'yes' to fix this :)

@TrySound
Copy link
Member

@adam-h Okay. I need some time.

@jonny-improbable
Copy link

@TrySound is there an update / workaround for this issue? It's preventing us from upgrading to v8 of postcss-import.

Thanks.

@jgthms
Copy link

jgthms commented Mar 1, 2016

+1

@adam-h
Copy link
Contributor Author

adam-h commented Mar 2, 2016

@jonny-improbable I only have a very hacky workaround, which only works for my codebase without modifications.

In my case this error results in variables not getting replaced in the resulting CSS. So I created a grunt-exec task to re-build the CSS until there are no variables output.

    exec:
      css_hack:
        command: ->
          variable_re = /(\$base-font|\$keyword-highlight|\$emphasis-font)/g
          match = variable_re.exec(grunt.file.read("path/to/output.css"))

          if(match)
            "./node_modules/.bin/grunt css"
          else
            "echo 'OK'"

Then made by build target run css then exec:css_hack

@MoOx
Copy link
Contributor

MoOx commented Mar 9, 2016

@TrySound any clue?

@TrySound
Copy link
Member

@MoOx Sorry, don't have a time for realization. But I have some ideas. Will try them asap.

@MoOx
Copy link
Contributor

MoOx commented Apr 1, 2016

@adam-h what do you think about v7 behavior? We might consider reverting to this version if fix is not doable nor easy. New v8 is really weird and seems more complicated to me.

@MoOx
Copy link
Contributor

MoOx commented Apr 4, 2016

@TrySound are your ideas still around? :D

@adam-h
Copy link
Contributor Author

adam-h commented Apr 4, 2016

@MoOx Can't easily try out v7, as my codebase is using the updated resolve function at the moment

@adam-h
Copy link
Contributor Author

adam-h commented Apr 12, 2016

@TrySound Any update on this?

Found some more edge cases that fail the build randomly that are trickier to workaround. Depending on the timescale I may revert to v7 and rework my build code slightly as @MoOx suggested and see how that works.

@adam-h
Copy link
Contributor Author

adam-h commented Apr 29, 2016

@TrySound Did you get anywhere on this? I may have some time in the coming weeks to have a poke at it

@wc-matteo
Copy link

In my case this error results in variables not getting replaced in the resulting CSS.
#169 (comment)

Same problem here.

Considering that I don't need any new feature introduced in v8, would reverting to v7 be a viable option? Is it fully compatibile with the current version of postcss? I checked and this issue doesn't appear there (disabling skipDuplicates also solves the issue, but it is less than ideal).

@MoOx
Copy link
Contributor

MoOx commented May 11, 2016

would reverting to v7

Totally.

In v8, I think there is a change with import scoped by media queries, but nobody use this anyway.

Btw, if I don't get any update within a month or so, I will revert to v7 implementation (because I don't have time to maintain/fix v8).

This issue has been open for too long.

@MoOx
Copy link
Contributor

MoOx commented May 11, 2016

FYI #210

@wc-matteo
Copy link

So this @import "css/bar.css" (min-width: 25em); is v8 only?

If so, I don't see any good use cases for it (in my workflow).

@MoOx
Copy link
Contributor

MoOx commented May 11, 2016

It's not v8 only, but I think in some use cases, nested import with mq where failing. @TrySound can say more about this.

@longlho
Copy link

longlho commented Sep 27, 2016

@MoOx any update on reverting to v7?

@MoOx MoOx merged commit 344528c into postcss:master Nov 2, 2016
@MoOx
Copy link
Contributor

MoOx commented Nov 2, 2016

Merging to move forward with this mess

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 2, 2016

@MoOx See #168 (comment)

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.

8 participants