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

replace rework css with postcss #78

Merged
merged 1 commit into from
May 15, 2018

Conversation

Tidyzq
Copy link

@Tidyzq Tidyzq commented Feb 10, 2018

Hi, I just came across the problem mentioned in #28 , and i found a lots of issues with the same problem that caused by rework, so i simply fork this repo and replace rework with postcss(which used by css-loader). However, just one second before i send this PR, i noticed that there is a branch trying to have multiple engines, so i wonder should i send you another PR to write a postcss engine?

In my point of view, it is 4 years since the last update of rework, so maybe we should simply drop it. Also, as css-loader is also using postcss as its parser, it can ensure that the CSS Module syntax won't cause any issues any more.

@bholloway
Copy link
Owner

bholloway commented Feb 12, 2018 via email

@stereokai
Copy link

@Tidyzq @bholloway just a quick update: this loader is amazing, it solved many problems for my team since we transitioned to webpack a few months ago. However, we decided to switch to Tidyzq's fork because rework is old and really unnecessary considering the ubiquity of PostCSS. Plus, we need SCSS with CSS Modules today, and a broken resolve-url-loader is unfortunately in the way of the modern syntax. Thanks for understanding.

@tpraxl
Copy link

tpraxl commented Mar 21, 2018

@bholloway did you have a chance to look at this PR? As far as I get it, this would resolve #28. If I get it right, then it would also make this workaround for tailwind css unnecessary (see heading "Laravel Mix"). The tailwind css workaround causes side effects for sass users. But maybe I'm not getting the real problem. Is there anything I can do to help?

@bholloway
Copy link
Owner

@stereokai Thanks for the kind words. I appreciate your position and yes there has been some neglect here. In development have favoured not breaking existing users and lack of tests has made change hard. I am trying to remedy that but definitely do what you have to for your build.

@tpraxl IMHO this needs to be on an opt-in basis for existing users to manage their change. I have a multiple-engines branch where I hope to integrate this work. This week I am making a concerted effort on tests there. I will then try to integrate the postcss engine. Thanks to @Tidyzq for the PR that kick started this overdue work.

As always I appreciate comments, suggestions, prodding, etc.

@stereokai
Copy link

Thank you for your consideration! I wish I could offer help, but as of now I am deep over my head in development anyway. So I'll just wish you good luck and have fun :)

@futhr
Copy link

futhr commented May 6, 2018

This PR work fine and solve current issues, it just need rebase and happy merge ;)

@bholloway
Copy link
Owner

I do need to get this in the mainstream release but I will need some more time. I am happy to consider ways of distributing this PR in the interim.

Rather than installing from a github commitish perhaps we could just release this under another dist tag. That would buy me some more time. @futhr would that help?

@futhr
Copy link

futhr commented May 7, 2018

@bholloway Yes, the PR now lives in a forked master branch so that sounds fine as a temporary solution, thanks :)

@bholloway bholloway changed the base branch from master to experiment/postcss May 15, 2018 08:09
@bholloway bholloway force-pushed the experiment/postcss branch from 7105d8a to b56f188 Compare May 15, 2018 08:29
@bholloway bholloway merged commit 527b91d into bholloway:experiment/postcss May 15, 2018
@bholloway
Copy link
Owner

I have published as resolve-url-loader@experiment-postcss. I had to bring in a few more commits but passes the (basic) e2e tests that I have thus far written.

Thanks again to @Tidyzq. Sorry for the delay.

@stereokai
Copy link

Noice! Thank you @bholloway! Is there a plan to get this into master some day soon?

@bholloway
Copy link
Owner

Please refer to #97 as version 3 uses postcss by default.

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.

5 participants