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

Implement Index a default file #2456

Merged
merged 8 commits into from Mar 9, 2018
Merged

Implement Index a default file #2456

merged 8 commits into from Mar 9, 2018

Conversation

john681611
Copy link

@john681611 john681611 commented Jan 23, 2018

Attempting to fix #690

Simple implementation that if no file in current level is found it looks if it's a directory and searches for a valid index file inside.

Though I have the tests working I am unsure of how to run the code locally in a manual test.

Working with:
@liamshaunsmith
@mariann013

See sass/sass-spec#1206

@nex3
Copy link
Contributor

nex3 commented Jan 23, 2018

Thanks for the contribution!

Because we're in the process of deprecating Ruby Sass, we aren't currently adding new features to Ruby only. If you're willing to add this behavior to Dart Sass as well and add cross-platform tests to sass-spec, then we can land it in both places at once.

@john681611
Copy link
Author

Ok I've got zero experience in dart so I will have a go

@nex3
Copy link
Contributor

nex3 commented Jan 24, 2018

Give it a shot! Dart is pretty easy to learn.

@nex3
Copy link
Contributor

nex3 commented Jan 26, 2018

Same comments as on the Dart PR: if the import ends in .scss or .sass it shouldn't look for the index, and the tests aren't necessary here.

@@ -154,7 +154,9 @@ def find_real_file(dir, name, options)
[Sass::Util.cleanpath(full_path).to_s, s]
end
end.flatten(1)
return if found.empty?
if found.empty? && _extension?(name) && File.directory?("#{dir}/#{name}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what _extension?(name) is doing here... are we only checking for an index file if name has an extension? That seems wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We figured it would be more readable than split(name)[2].nil? I'll switch back

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant more "why do we need to check for an extension at all" than "why is it factored out into a private method"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of your above comment that if it has an extension in the name we shouldn't look at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant a .sass or .scss extension. If we disallow any extension, this spec will fail (and in fact I'm not sure why it isn't failing already...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I was being dumb here... I didn't realize that this was using the split() method defined below that already checks for the correct file extensions. Your original code was fine; I'll revert b196d9c.

john681611 and others added 4 commits January 30, 2018 12:00
This reverts commit b196d9c.

This wasn't necessary, I just misunderstood the existing behavior.
@nex3 nex3 changed the base branch from stable to next February 2, 2018 22:57
@nex3 nex3 merged commit ad7760a into sass:next Mar 9, 2018
@john681611 john681611 deleted the FESK-919-Default-sass-path branch August 2, 2018 15:00
@AndrewBogdanovTSS
Copy link

Any info on when this feature might be released?

@john681611
Copy link
Author

I believe its released in https://sass-lang.com/dart-sass, ruby-sass is being deprecated so I'd look into moving to that.

@nex3
Copy link
Contributor

nex3 commented Dec 4, 2018

This was released as part of Ruby Sass 3.6.0.

@john681611
Copy link
Author

This was released as part of Ruby Sass 3.6.0.

Something I have been wondering is did this make it into lib-sass or is that another separate piece of work I need to do?

Basically, we need a web pack sass-loader that supports dart-sass and does de-duping.

@nex3
Copy link
Contributor

nex3 commented Dec 6, 2018

Something I have been wondering is did this make it into lib-sass or is that another separate piece of work I need to do?

It looks like this landed in sass/libsass#2614.

Basically, we need a web pack sass-loader that supports dart-sass and does de-duping.

The Webpack Sass loader support Dart Sass now. You just need to pass implementation: require("sass") to it.

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.

None yet

3 participants