Skip to content
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

Update Dependencies #155

Merged
merged 4 commits into from
Jun 12, 2019
Merged

Update Dependencies #155

merged 4 commits into from
Jun 12, 2019

Conversation

shrugs
Copy link
Contributor

@shrugs shrugs commented Jun 12, 2019

  • react-scripts -> 3.x
  • ledger libs -> 4.61.0
  • lodash -> lodash-es

this also happens to bring our audit results back down to 0 vulnerabilities 🎉

I updated react-scripts to 3.0 following the migration guide

I removed babel-polyfill, intending to replace it with regenerator-runtime/runtime, according to the migration guide but found that I didn't get an error when loading the ledger page (afaik, the error would have been thrown immediately because of the implicit global dependency on the regenerator-runtime features).

I'm going to fully test the ledger flow on Monday using the office ledger, unless you happen to have one, @Fang-.

I can't tell if this is related to this PR or not, but my hotreloading also broke in firefox, and this seems like the relevant issue.

I also added an analyze script that shows bundle sizes. The azimuth-js check.js file takes up a massive amount of space, so probably worth debugging that. Likewise for urbit-key-generation.

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

lodash -> lodash-es

Can I get a tldr on this? Sounds like a different thing. (Though I get that lodash is just utils, so this is probably fine.)

I have a Ledger so can test this. Was already on my todo list because of #140. (:

The azimuth-js check.js file takes up a massive amount of space, so probably worth debugging that.

Interesting! It's pulling in all the other contract-specific libs, but I'd hope that gets deduplicated or something?

@shrugs
Copy link
Contributor Author

shrugs commented Jun 12, 2019

lodash

lodash-es is the same as good ole' lodash, but is just an ES module. by using an ES module instead of the normal commonJS, the bundler should be able to avoid bundling functions that we don't use, but after actually testing, that doesn't seem to be the case. not a big deal at the moment, so I've reverted to normal lodash

Interesting! It's pulling in all the other contract-specific libs, but I'd hope that gets deduplicated or something?

it doesn't have a chance to get deduplicated because we're just pulling in the /dist/ compiled blob. If we exported an ES module I think webpack might dedupe? Not entirely sure

@Fang-
Copy link
Member

Fang- commented Jun 12, 2019

puts a dollar in the js-ecosystem-headache jar

Merge at will!

@shrugs
Copy link
Contributor Author

shrugs commented Jun 12, 2019

@Fang- let me know if this branch works with ledger? just in case, can you also rm -rf ./node_modules and then npm install to get a clean slate?

@Fang-
Copy link
Member

Fang- commented Jun 12, 2019

I always burn node_modules after doing checkouts. Been bitten by that too many times already!

It was initially failing silently, but after updating my Ledger firmware it behaves normally. So, all good!

@shrugs
Copy link
Contributor Author

shrugs commented Jun 12, 2019

@Fang- great! merged! did you test chrome, specifically? #140 seems chrome related

@shrugs shrugs merged commit 52cd2b2 into mino Jun 12, 2019
@shrugs shrugs deleted the fix/update-deps branch June 12, 2019 23:19
@Fang-
Copy link
Member

Fang- commented Jun 12, 2019

I did now! Can authenticate just fine.

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