Skip to content

Conversation

@43081j
Copy link

@43081j 43081j commented Jul 12, 2021

This PR contains a:

  • metadata update

I suppose its "metadata", manifest updates.

Motivation / Use-Case

mem pulls in 3-4 deep of dependencies for something we can achieve with a single file of code.

this just drops it for a more sensible, dependency-free package that does the same job and does it faster too.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just drop this package in favor built-in function, it is not hard to implement

@43081j
Copy link
Author

43081j commented Jul 12, 2021

works for me.

we only use it here so a map of string -> url would do i think.

mind taking a look at my last commit?

tbh we probably don't even need to cache it anymore, creating an instance of the newer URL class isn't likely to be slow

weirdly the one test it caused to fail was expecting an error for a malformed URL, but the URL it contained wasn't actually a malformed URL (according to the whatwg implementation). so i updated it to actually be malformed

if you need to support a version of node that hasn't got URL (not sure when it was introduced), i can change it to use the deprecated parse again

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #968 (fd58a05) into master (0148fd9) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #968 +/- ## ========================================== + Coverage 97.90% 97.95% +0.04%  ========================================== Files 10 10 Lines 287 293 +6 Branches 100 101 +1 ========================================== + Hits 281 287 +6  Misses 6 6 
Impacted Files Coverage Δ
src/utils/getFilenameFromUrl.js 97.82% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0148fd9...fd58a05. Read the comment docs.

@43081j
Copy link
Author

43081j commented Jul 20, 2021

have rebased onto master and simply dropped the dependency, using a map instead.

let me know if it makes sense / any changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants