Simplify API for streaming and fix unmatchable bug

theKashey / used-styles

📝All the critical styles you've used to render a page.

MIT License

138 stars 9 forks source link

Simplify API for streaming and fix unmatchable bug #16

Open 7rulnik opened 4 years ago

7rulnik commented 4 years ago

We can simplify API for users by hiding pipe inside of createStyleStream/createCriticalStyleStream. So users will just wrap React stream with our function. So the whole render process becomes simpler because we don't need multistream dep and also it's easier to catch errors because now we forward error from React stream.

I don't understand your idea with block rendering (I don't understand what headerStream stands for) so it's kinda hard to update usage in this block.

Also, I can make it with backward capability but not sure that it worth it.

And there is a problem with extractAllUnmatchableAsString. It didn't wrap css into style tag. I fixed it in this PR, but I can create another one for it.

theKashey commented 4 years ago

👍 I like how easier is use your API
👎 I don't like breaking API change and less flexibility - the same could be done by exposing one command (like "combine")

PS: It would be very good to have a bug fix a separate PR to publish it without any delay.

theKashey commented 4 years ago

Hey! I've updated underlaying tools and updated tslint/prettier caused some conflicts :( I've also fixed the extractAllUnmatchableAsString bug you've discovered 🙇

7rulnik commented 4 years ago

You stole my contribution :D Okay, I will rewrite it to combine function as we discussed

sayjeyhi commented 4 years ago

@theKashey I thought @7rulnik is right about simplifying of API without using multistream. as it seems it did not maintained any more. in other hand it is hard to start using used-styles for now cuz of complexity.

sayjeyhi commented 4 years ago

I thought there is not much usage of this library for now and breaking API change is not big deal.

7rulnik commented 4 years ago

We discussed a bit this idea with @theKashey. He wants to save flexibility of api (for example, you can pipe into used-styles directly). I agree with him so I will introduce combine function that will simplify usage of used-styles.

Somethin like that:

combine(reactStream, usedStylesStream)

7rulnik commented 2 years ago

@theKashey looks like it could be revived :D Also maybe we need some changes to adopt it for React 18

theKashey commented 2 years ago

👋 hey, there is a good time to break API in some way to support https://github.com/theKashey/used-styles/issues/40. That would require internal API to become a little bit more abstract so one can build more specific code on top of.

So can you clarify what exactly you are looking for and let's do it.

some changes to adopt it for React 18

I've used used-styles for React 18 in "old good rendering mode" with no issues (and there could be none), but I am really not sure how to support the "new rendering mode". A broken test would be appreciated.