arpadHegedus / postcss-node-sass

A PostCSS plugin to parse styles with node-sass
MIT License
23 stars 16 forks source link

2.0.0 #2

Closed jonathantneal closed 6 years ago

jonathantneal commented 6 years ago

Resolves #1

jonathantneal commented 6 years ago

Hey @arpadHegedus, would you have some time this week to review this PR? I won’t be offended if you ultimately aren’t interested in combining our efforts. I also understand that open source is often unrewarded, and our income jobs don’t afford us time to contribute to these other projects. Please let me know if there is anything I can do to help. 😄

arpadHegedus commented 6 years ago

I do want to collaborate, but I'm on a big project currently and will finish next week. I've started implementing some of the stuff in your repo merging it in. But I also won't be offended if you want to publish yours separately. I'm sorry to be slow to respond.

jeffjewiss commented 6 years ago

Hi @arpadHegedus, could you please reconsider accepting this pull request?

The current version of postcss-node-sass doesn't seem to work. I was able to get @jonathantneal's version working with a project of mine.

This PR adds tests, linting and a number of other useful features as well as making the development of the plugin easier. @jonathantneal has a proven track record of releasing quality work in the Postcss ecosystem as well.

I don't see how his changes make the plugin more complicated. They keep it as simple as your version, but add much needing tooling and support.

Your decision to close both the issue and this PR is confusing since you didn't actually merge in any of his commits.

Again, I ask you to reconsider merging this pull request as the plugin would still be under your name, but everyone would benefit from these changes.

Thanks

arpadHegedus commented 6 years ago

Hi @jeffjewiss

Can you describe the error you have? You say the plugin doesn't seem to work.

I understand your concern of having no test and linting yet, I will address these in a future release. But the Pull Request was not really a commit to my repo, it was a completely new set of code. As I have mentioned before, Jonathan is still free to add his own plugin, there are instances of postcss plugins that generally do similar or the same tasks but they are separate.

I will double check the current version and try and add your requests in a future release.

jeffjewiss commented 6 years ago

I don't really understand your stance on this.

Hearing that your answer addressing testing and linting is to add them in a future release is frustrating when the code for them is right here in a pull request.

I also don't understand your statement that "the Pull Request was not really a commit to my repo". That's exactly what it is, a commit to this repo. If you disagree with the code style or use of the new .mjs module syntax, or anything else really, you could have left feedback for it to be updated. However, you provided no feedback and simply closed the request. This isn't a great way to get adoption for a plugin or contributions either.

I realize you mentioned that Jonathan is free to add his own plugin, but that's a cop out. I'm sure he's well aware he could publish it under a different name. However, he was willing to work together and contribute his work that he spent time on to this project, which is yours and under your name.

I'm sad that this is your attitude towards collaboration and a contribution like this. I'd love to help with a plugin like this, but can't see that happening if this is your response to other's work.

arpadHegedus commented 6 years ago

@jeffjewiss I agree with most of what you say. I didn't mean to be rude or uncollaborative. What I meant was that the pull request was initially its own project and accepting it would have simply overwritten mine with no trace left of the original repo or code style, etc. I think the pull request has a lot of good ideas and started building them into the main base. It is not finished yet as I have very little time at the moment, but I wanted to "manually do it" so that I also learn from it while keeping the same style of code as well. I will try and implement the ideas and tooling along with the rest of this thread's suggestions and probably try and stay away of pushing my changes uncompleted like this next time.

jonathantneal commented 6 years ago

@arpadHegedus, I think you were nice and straight forward. Seriously, a lot of the time people just don’t respond.

@jeffjewiss, I’ve published the alternative version.

https://github.com/jonathantneal/postcss-sass

Both plugins let you use node-sass as part of the plugin chain, which is the most important part, and both preserve source maps. @arpadHegedus’s is less complex, which means it will be easier to maintain. Mine is more complex, at the benefit of letting you watch for changes to Sass imports (when you use --watch, PostCSS Loader, webpack, etc.). Not everybody needs that, but I do. :)

There is currently an issue with merge-source-map that prevents source maps from merging cleanly when Sass imports are involved. My plugin also includes a workaround for that. Over time, I hope the plugin will become less complex.

jeffjewiss commented 6 years ago

@arpadHegedus thanks for responding to my previous comments, I appreciate you taking the time to do so. I understand that it's ultimately your project and package and I respect that you'd like to keep the code style and manually make the changes to learn.

@jonathantneal thanks for taking the time to provide your perspective.

Cheers.

arpadHegedus commented 6 years ago

@jonathantneal If you agree, I have added you as a contributor to the new version as I've learned so much from your code and implemented some of it.

If you are not happy, I can remove you from package.json.

Thanks.