-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Compare the sizes of multiple packages and analyze package.json #441
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/styfle/packagephobia/b1pw5z8b5 |
Hi Jonah, Thanks for the PR! I was hoping someone would implement this feature 😄 I'll need to spend some time reviewing this feature before merging but so far it looks great! 🥇 |
Awesome thanks! Just did a first pass on your initial comments, will come back for the remaining ones soon. |
let data: any[] = []; | ||
req.on('data', chunk => data.push(chunk)); | ||
req.on('end', () => { | ||
const [packageString] = data.toString().match(/{[\s\S]+}/) || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this regex? Can you instead use JSON.parse(data.toString())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the data comes through as multipart/form-data
, which includes the content boundaries (https://www.w3.org/TR/html4/interact/forms.html#h-17.13.4.2). This seems like one lightweight way to parse the contents out, but definitely open to better options if you have any thoughts.
flexDirection: 'column', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use import PageContainer from '../components/PageContainer';
here from PR #450
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, now that I think about it. We don't need a separate page for parse failures. We need to modify the 500 error page to print a user friendly error.
Perhaps create something like class PageError extends Error
that we can then detect in the router to determine if the error should be rendered to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wonderful, great job! 🏅
Thanks for taking the time to make this PR! 🤩
Thanks so much for your help and patience with this! |
Is this already usable on the front-end of bundlephobia? |
Yes |
@styfle It's strange, it the UI on the main website doesn't indicate we can compare packages at all: Even if I type something like: Also, when tapping enter on any auto-completed package name, it gives the page for just that package, loosing any other packages I had type before that with comma's. Is there anything I can perhaps help with to improve this UI? I'm a Vue developer I can help. |
@mesqueeb I think you're confused. This repo is Package Phobia, not Bundle Phobia. Here's the working page: https://packagephobia.now.sh/result?p=marked,markdown-it Read more about the similar tools in the readme here: https://github.com/styfle/packagephobia/blob/master/README.md#how-is-this-different |
This PR adds package comparison using commas in the search bar and the ability to upload a
package.json
from the index page. Fixes #133This is very "minimum viable implementation" — there's definitely room for improvement, especially on the frontend of the comparison page, but I wanted to put this out there as a possibility for the implementation of the core functionality. Happy to chat about and reconsider any of the design decisions here!