-
Notifications
You must be signed in to change notification settings - Fork 947
truncate_with_package_name sometimes grabs the wrong name #350
Comments
Not knowing much about Any thoughts from our JS devs? |
It was done this way to avoid external dependencies or firing up an interpreter.
Using a complex RegExp supporting all cases will surely be cpu intensive (~2ms for bracket depth=1 and the exemple file). So at this point I'll recommend using, if present: jq, node, python or the current method, to decode the json. |
Thanks for the expert opinion, @V1rgul! Based on this, what, if any, changes would you recommend to our existing segment code? |
I think we should use jq.
So the question stands with, do we still use the current method as a fallback ?
|
This is very much an edge case, this is the first repo out of hundreds that I've seen done this way. It might not be with fixing if the performance is way worse. I agree node.js is too slow |
Thanks for the testing, @V1rgul. I agree with you @fracmak that `node.js' is clearly too slow. I'm always very hesitant to add additional dependencies / requirements for a working P9K installation, but it would be good to somehow inform users that installing Here's my suggestion: When P9K initializes, use the segment_in_use utility function to test for whether or not this segment is on. If it is, print a one-time warning that recommends the use of Thoughts? |
I ran some tests locally and jq is definitely faster than the current awk/grep combo
|
Just a quick idea: local pkgFile="package.json"
local packageName=$(jq '.name' ${pkgFile} 2> /dev/null || node -e 'console.log(require(process.argv[1]).name);' ./${pkgFile} 2>/dev/null || cat "./${pkgFile}" 2> /dev/null | grep -m 1 "\"name\"" | awk -F ':' '{print $2}' | awk -F '"' '{print $2}' 2>/dev/null) It would try the jq first, then fail gracefully, try node, and at last do it with grep and awk. That way @fracmak Awesome testing! 👍 |
Actually, my understanding is that @dritter and @V1rgul are suggesting the same thing. @V1rgul's point, I think, was that by failing gracefully, it enables users to not have to install I do want to fail gracefully because I don't want to add a dependency, but I also want to make users aware that installing |
@fracmak - I'd like to move ahead with getting this merged. Let's document that @fracmak @V1rgul - What needs to happen on this branch to finalize it for merge? |
The exemple dritter commented should be ok, it just needs to be integrated into the source |
Hi all! I created PR #411 for this. Could you confirm that the fix works as expected? |
Merged #411 into |
If the repo "name" isn't the first name in the package.json file, it'll print out the wrong name.
Example:
https://github.com/gajus/babel-plugin-react-css-modules/blob/master/package.json
spits out "Gajus Kuizinas" as the project name instead of "babel-plugin-react-css-modules"
The text was updated successfully, but these errors were encountered: