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

Play well with Custom Syntax #245

Merged
merged 2 commits into from
Nov 23, 2016
Merged

Play well with Custom Syntax #245

merged 2 commits into from
Nov 23, 2016

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Nov 16, 2016

This is still a WIP. Will most likely need rebased when I'm done.


Warning: This is a weird approach to solving this problem.

Perhaps this approach is naive; but it should be pretty well bulletproof. It is based on the (correct 😛 ) assumption that if a parser can parse the input, it is the correct parser.

If the file is .sss, it tries that parser first. For all other files (or if SugarSS parsing fails) it tries the user-passed parser (if available). If that fails, it tries the default parser.

The reason I chose this approach is that we have no reliable way of telling if result.opts.parser is a CSS-compliant parser. See #242 for more background.

The main problem with this approach is error handling/reporting. Currently, it gives the user the error from the default parser. In the case of a malformed .sss import, this is quite unhelpful. Not sure what is the best route to improve this.


This is by no means a polished PR. I may end up totally rewriting the implementation. Anyway, it does provide a good test suite for how things should behave.

CC: @ai @MoOx

@RyanZim RyanZim added this to the v9.0.0 milestone Nov 16, 2016
@RyanZim RyanZim mentioned this pull request Nov 16, 2016
@ai
Copy link
Member

ai commented Nov 16, 2016

Awesome! You did big work in short time :).

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 16, 2016

@ai @MoOx Made a change so that only the SugarSS parser is tried when the extension is .sss. That should fix some of the incorrect errors.

You're still stuck with bad errors if you're using a css-compliant parser and make a syntax error. You will get the error message from the default parser. 😢 Thoughts?

@ai
Copy link
Member

ai commented Nov 17, 2016

Looks like good. But sorry, I have no postcss-import case to test it.

@MoOx
Copy link
Contributor

MoOx commented Nov 17, 2016

I can't help, I never used anything that raw css.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 17, 2016

I don't use custom parsers either.

Do you think this error problem is big enough to block this PR? We can always change error reporting later.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 19, 2016

@ai @MoOx Not sure if it's worth adding support for .scss file extensions or not. What do you think?

@ai
Copy link
Member

ai commented Nov 19, 2016

Sure, complicated question. I afraid that user will try to import real SCSS sources (not just with // comments).

Let's start without it, and add it later if user will ask for it (at least we have use cases from them).

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 19, 2016

@ai That's what I was thinking.

I'll rebase this and then leave it for final review before merging it.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 21, 2016

@ai @MoOx Anything else that needs changed? If not, I will merge this.

@ai
Copy link
Member

ai commented Nov 21, 2016

LGTM

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 23, 2016

OK, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants