Skip to content
This repository was archived by the owner on Nov 4, 2023. It is now read-only.

Rewrite async API to use promises #16

Closed
wants to merge 5 commits into from
Closed

Rewrite async API to use promises #16

wants to merge 5 commits into from

Conversation

andy128k
Copy link
Contributor

Related to item 2 in #12

@lydell
Copy link
Owner

lydell commented Mar 31, 2020

Hi! Wow, I did not expect anyone to come and do this! Amazing!

I’m curious – what’s your motivation to dive into this shitty code base? Just for “fun”? Do you use source-map-resolve and miss the async API? How far would you like to take the refactor?

@andy128k
Copy link
Contributor Author

Hi

I was bored. And then yarn showed me this:

warning babel-jest > @jest/transform > jest-haste-map > sane > micromatch > snapdragon > source-map-resolve > [email protected]: https://github.com/lydell/resolve-url#deprecated
warning babel-jest > @jest/transform > jest-haste-map > sane > micromatch > snapdragon > source-map-resolve > [email protected]: Please see https://github.com/lydell/urix#deprecated

...

I do not use source-map-resolve directly.

If you still have such tasks I can try to do them too.

@lydell
Copy link
Owner

lydell commented Mar 31, 2020

If your goal is to get rid of those deprecation warnings, then you need to know:

  • source-map-resolve 0.6.0 already got rid of the deprecated dependencies.
  • You’d spend your time better trying to fix that dependency chain. For example, the latest version of micromatch does not even depend on snapdragon anymore.

@andy128k
Copy link
Contributor Author

My goal was to entertain myself. I don't care about those warnings that much.

@lydell
Copy link
Owner

lydell commented Mar 31, 2020

Ok!

Here are some ideas I’ve played around with:

  • The test suite covers a lot of cases, but is horrible to work with. I’d like to structure the tests in a better way, and use Jest.
  • Maybe we can get rid of the sync functions since we’ve got await these days.
  • Nicer API, that works well with TypeScript. Maybe write the whole thing in TypeScript?
  • ESLint instead of JSHint.
  • Prettier.
  • Fixing Incorrect path resolving on Windows with multiple disks #9. Windows support is super not thought through.
  • etc.

@andy128k
Copy link
Contributor Author

Preview of conversion to Jest: 515124f

Like this?

@andy128k
Copy link
Contributor Author

If you plan to get rid of sync functions it is better to do this before conversion to Jest. :)

@lydell
Copy link
Owner

lydell commented Apr 1, 2020

Yeah, something like that.

Let’s get rid of the sync functions then!

Also, if you want to go the TypeScript route that’s probably good to do before tests, too.

This was referenced Apr 1, 2020
@lydell
Copy link
Owner

lydell commented Apr 2, 2020

I think it would be easier to merge all your PRs into one giant update-all-the-things PR. What do you think?

@andy128k andy128k mentioned this pull request Apr 2, 2020
@andy128k
Copy link
Contributor Author

andy128k commented Apr 2, 2020

#20 All changes are there

@lydell
Copy link
Owner

lydell commented Apr 3, 2020

Closing in favor of #20.

@lydell lydell closed this Apr 3, 2020
@andy128k andy128k deleted the modernize branch April 3, 2020 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants