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

Switch to an async api #71

Closed
wants to merge 4 commits into from

Conversation

SleepWalker
Copy link

refs #43

It was a mistake to say, that everything will fit in fixUrl :)

Nevertheless, I have finished with some draft implementation! And successfully tested it on my project. But there are, probably, quite a lot of changes...

I can reduce the amount of code, but I have some questions:

  • according to the old logic valueCallback will return oldUrl in two cases: when the url should not be processed (isUrlShouldBeIgnored) and when UrlProcessor returned an empty url. Is the second case expected behavior or it is a bug? For now I've leaved this logic as it is and added warning for the case, when UrlProcessor returns bad value.
  • For the flow with promises, I need to do two String.replace calls per url. I think, that this extra call is not expensive, but I can be wrong. So I've wrote separate code for sync and async replacements for now. If it is ok, I can wrap all UrlProcessor results in Promises and drop sync logic to reduce the amount of code.

And another one question: should I polyfill Promise for an old Node.js?

@niksy
Copy link
Contributor

niksy commented Oct 3, 2016

What is the status of this PR? Can we maybe help in getting this PR merged (e.g. testing various use cases)?

@SleepWalker
Copy link
Author

@niksy I am using it for my projects and waiting for any feedback here.
@MoOx can you take a look?

@RReverser
Copy link

This is great. Is anyone still looking into this PR? Being able to handle asynchronous external deps would be very helpful.

@niksy
Copy link
Contributor

niksy commented Aug 8, 2018

@RReverser unfortunately no, and it's a real shame :( In the mean time, I've found that https://github.com/sebastian-software/postcss-smart-asset is fork of this module but has async support so you can try that. I've been using it myself and it works.

@sergcen any chance someone can look at this and help us get this merged? Async API would be really helpful.

@jonathantneal
Copy link
Member

This branch is now out of sync with master to the point of conflicts. If there isn’t new activity soon, I would say this should unfortunately be closed.

@niksy
Copy link
Contributor

niksy commented Mar 30, 2019

It would be real shame if this is missed opportunity. I think postcss-url is one od the most used PostCSS plugins and having async API (by default, but that's another story) is must have today. Can we do something to merge this?

@MrMeison
Copy link

MrMeison commented Apr 2, 2019

Don't panic, I repairing this PR :)

@SleepWalker
Copy link
Author

@MrMeison There was no comment from maintainers for two years. There is no sense to write any code string, till we know, that this PR will be accepted.

@sergcen
Copy link
Collaborator

sergcen commented Apr 2, 2019

PR will be accepted after @MrMeison will fix conflicts

@MrMeison
Copy link

MrMeison commented Apr 2, 2019

goto #134

@MrMeison MrMeison mentioned this pull request Apr 2, 2019
@sergcen
Copy link
Collaborator

sergcen commented Apr 16, 2019

published 9.0.0 with beta dist-tag

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

6 participants