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

exports() and index() function #5932

Closed
HamptonMakes opened this issue Oct 14, 2014 · 17 comments
Closed

exports() and index() function #5932

HamptonMakes opened this issue Oct 14, 2014 · 17 comments

Comments

@HamptonMakes
Copy link
Contributor

Hi Foundation!

So, with your current exports() function it seems that you are testing if index() is equal to false. And, #5811 exists, which is talking about the loss of 3.4 compatibility in favour of libsass and 3.2-3.3 compat, but that seems to be focusing on rails and on the !global change, so I've opened this ticket to discuss the index() issues.

In Sass 3.4 and libsass 3.0 (pending release), index() returns null when no value is found, whereas in previous versions, it returned false. That is the source of the current error and it's possible to give support for all versions at once with the following code.

$modules: () !default;
@mixin exports($name) {
  $module_index: index($modules, $name);
  @if (($module_index == null) or ($module_index == false)) {
    $modules: append($modules, $name);
    @content;
  }
}

Foundation has been such a great supporter of libsass, that we didn't want to release 3.0 without solving sass/libsass#538.

I'll make a pull request with this change to support all recent versions of sass and libsass.

@gakimball
Copy link
Contributor

There's another issue, which is the !global keyword. Sass 3.4 needs it, because otherwise it will create a new local variable for $modules every time, which messes up the logic of the exports() mixin. As I recall there was a PR to libsass to add a fake !global keyword, which sidesteps the issue until libsass implements the new variable scope changes. However, if Sass 3.2 sees it, it throws an error. You can't hide it behind an @if statement or anything like that.

That's a less pressing issue than the libsass thing, though, and also not something either of us can fix. Basically sass-rails is hardcoded to Sass 3.2, and while that crew is working on 3.3 support, there hasn't been a new release since August.

But we really appreciate y'alls wanting to work with us <3

@bdkjones
Copy link

I'm happy to confirm that the change above works on:

Libsass 2.x (master branch with the !global PR merged)
Libsass 3.0 RC4
Ruby Sass 3.4.5 (with a bunch of duplicate CSS; see below)

Tested with Foundation 5.4.6. The CSS looks 100% correct and the Foundation starter page is styled correctly in each case.

Update

I do see the !global issue @gakimball mentions. Without adding !global after the $modules: append(...);, Ruby Sass 3.4.5 duplicates a bunch of CSS and the resulting file is 15,000+ lines. With !global, that drops to ~6,000.

Libsass 3.0 RC4 handles things correctly, regardless of whether !global is present.

The last official release of Libsass 2.x does not recognize the !global keyword, but I've been using the master branch of Libsass from mid-July 2014, which has the PR for fake-global-handling merged. That version of Libsass compiles the same, regardless of the !global keyword's presence.

@bdkjones
Copy link

Do you guys have any idea what percentage of the Foundation user base is compiling with sass-rails? Unless it's a big number, I'm not sure holding back Foundation to support sass-rails is a good idea (especially if that team is going to constantly lag behind the official Sass releases by many months).

It is also possible (at least with Bower) to install older versions of Foundation. You could simply tell folks that want to use sass-rails to install version 5.4.6 because versions after that aren't yet supported by sass-rails. Again, this approach would work if the percentage compiling with sass-rails is small.

@gakimball
Copy link
Contributor

Rails support is definitely important to us (we're also a Rails house so we have an incentive to keep it working), but you're right that a fair compromise would be leaving Rails users at 5.4.6 for now, as long as our users understand not to upgrade.

I know RubyGems.org's statistics don't correlate to the size of your userbase, but our old zurb-foundation gem has 600,000 downloads, and foundation-rails has about 200,000. Granted, zurb-foundation is the gem we used before version 5.0, which came out almost a year ago.

@bdkjones
Copy link

Understood.

So what's the plan? I don't mean to pressure you guys; I'm just trying to decide how I should handle this situation in CodeKit. Basically, I need to know if you guys are going to update the official repo with this pull request (plus the !global) keyword in the immediate future. If so, I can ship Libsass 3.0 with CodeKit and simply tell my users to update Foundation.

If not, then I'll have CodeKit manually scan _functions and replace the exports mixin with @hcatlin's version plus the !global keyword. I'd prefer not to go this route because, to the user, it's black magic. Folks compiling Foundation with Libsass 3 or Ruby Sass 3.4 in CodeKit would find it works perfectly. But if they tried to use those same sass compilers to compile a fresh install of Foundation without CodeKit, it would fail. Not ideal.

It's about 30 minutes of work for me to implement the manual workaround, so that's not really a factor. But if you guys are going to issue an update with support for newer Sass, that's the best route for me.

@HamptonMakes
Copy link
Contributor Author

It seeeeems like we should have support to not barf on !global. At least, based on sass/libsass#409. Obviously, libsass 2.0 still won't like it. I'm not on a machine where I can double check it yet.

@bdkjones
Copy link

@hcatlin confirmed: Libsass 3 tolerates !global.

Sent from my iPhone

On Oct 14, 2014, at 15:49, Hampton Catlin notifications@github.com wrote:

It seeeeems like we should have support to not barf on !global. At least, based on sass/libsass#409. Obviously, libsass 2.0 still won't like it. I'm not on a machine where I can double check it yet.


Reply to this email directly or view it on GitHub.

@HamptonMakes
Copy link
Contributor Author

Seems like this is fixable via a change to Foundation, I will make a note of it in the release notes for 3.0, but we've held back 3.0 for 2 days now just waiting on this issue and it's not really something broken with libsass, per se. SO! That's my way of saying I'm greenlighting libsass 3.0 today and hopefully we can get this fix merged in soon.

@gakimball
Copy link
Contributor

Sorry for the delayed response, busy week.

My last question was going to be about node-sass, which is what we use to compile. How soon will 3.0 be working in there? I assume that crew needs to do a little work with their codebase to make sure it works properly, right? And once they upgrade, would there be a clean split between node-sass versions that use libsass 2.0 and 3.0, just so our dependencies are in order?

@HamptonMakes
Copy link
Contributor Author

We've actually been coordinating with them on this release, so as soon as this goes out the door, they will update. It's already changed to work with our RC's.

@gakimball
Copy link
Contributor

Nice. We can definitely commit to upgrading, but is it pressing that we upgrade right away as long as node-sass doesn't flip over for our existing users? 5.4 has been enough of a minefield in terms of compatibility changes (granted, our fault), it would be nice if we could do this for 5.5, which we aren't planning on releasing for another few weeks. That way we can tell Rails/3.2 users "don't go to 5.5", versus "don't go to 5.4.7... or was it .6?"

@HamptonMakes
Copy link
Contributor Author

The patch I sent works for libsass 2.0 and 3.0. And for Sass 3.2 and 3.3. It does not have the !global flag.

In 5.5, you can add the global flag which makes it work for libsass 3.0, and Sass 3.3 and 3.4.

On Thu, Oct 16, 2014 at 12:17 PM, Geoff Kimball notifications@github.com
wrote:

Nice. We can definitely commit to upgrading, but is it pressing that we upgrade right away as long as node-sass doesn't flip over for our existing users? 5.4 has been enough of a minefield in terms of compatibility changes (granted, our fault), it would be nice if we could do this for 5.5, which we aren't planning on releasing for another few weeks. That way we can tell Rails/3.2 users "don't go to 5.5", versus "don't go to 5.4.7... or was it .6?"

Reply to this email directly or view it on GitHub:
#5932 (comment)

@gakimball
Copy link
Contributor

Great! We'll plan on doing that then. Thanks so much for your help!

@gakimball
Copy link
Contributor

Merged in #5933. We'll see if there's time to put out a minor patch tomorrow, but we weren't planning on it so it might need to be next week.

@stryju
Copy link
Contributor

stryju commented Oct 17, 2014

left a small note there https://github.com/zurb/foundation/pull/5933/files#r19006447 :-)

@simonexmachina
Copy link
Contributor

Any idea when a new version will be published?

@gakimball
Copy link
Contributor

This specific change for libsass 3.0 should be out this week, and early next month we'll release 5.5, which will be 3.4-compatible and 3.2-incompatible.

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

5 participants