Skip to content

replace mem with better, smaller alternative #968

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

Closed
wants to merge 2 commits into from

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants