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

feat: update to ipfs-geoip v9 #2061

Merged
merged 15 commits into from
Nov 9, 2022
Merged

Conversation

whizzzkid
Copy link
Contributor

Closes #2055

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

I see that CI tests are failing, and the reason isn't obvious but it seems multiformats-ish.

Comment on lines 109818 to 109821
"multiformats": {
"version": "9.6.5",
"resolved": "https://registry.npmjs.org/multiformats/-/multiformats-9.6.5.tgz",
"integrity": "sha512-vMwf/FUO+qAPvl3vlSZEgEVFY/AxeZq5yg761ScF3CZsXgmTi/HGkicUiNN0CI4PW8FiY2P0OLklOcmQjdQJhw=="
"version": "9.9.0",
"resolved": "https://registry.npmjs.org/multiformats/-/multiformats-9.9.0.tgz",
"integrity": "sha512-HoMUjhH9T8DDBNT+6xzkrd9ga/XiBI4xLr58LJACwK6G3HTOPeMz4nB4KJs33L2BelrIJa7P0VuNaVF3hMYfjg=="
Copy link
Member

Choose a reason for hiding this comment

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

we might want to prevent this change because of #1965

Copy link
Member

Choose a reason for hiding this comment

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

specific dependency tracking issue: #2048

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sooo... actually we don't need that. The dependencies on ipfs-geoip are wrong. Fixed those here: ipfs-shipyard/ipfs-geoip#100

Copy link
Member

Choose a reason for hiding this comment

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

left a comment at ipfs/aegir#1104 (comment) and mentioned it in a peek at ipfs-geoip pr

@lidel lidel changed the title (feat) Updating to geoip@9 feat: update to ipfs-geoip v9 Nov 8, 2022
@lidel
Copy link
Member

lidel commented Nov 8, 2022

@whizzzkid mind retrying with ipfs-geoip v9.0.1 that includes ipfs-shipyard/ipfs-geoip#100?

If we can land this PR before Thursday, then we could make a new ipfs-webui release and include it in Kubo 0.17-rc1 :)

@whizzzkid whizzzkid temporarily deployed to Deploy November 9, 2022 18:37 Inactive
Copy link
Contributor Author

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

ready for review @lidel @SgtPooki

if (!Array.isArray(rule.exclude)) {
rule.exclude = [rule.exclude]
}
PURE_ESM_MODULES.forEach(module => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since pure ESM modules like ipfs-geoip are already built, we need to let babel know that this can be skipped for transpilation.

function modifyBabelLoaderRuleForTest (rules) {
return modifyBabelLoaderRules(rules, rule => {
if (rule.options && rule.options.plugins) {
rule.options.plugins.push('istanbul')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this also gives us an opportunity to inject instrumentation in a much more cleaner way.

} else if (rule.oneOf) {
const nestedRules = modifyBabelLoaderRule(rule.oneOf, false)
foundRules.push(...nestedRules)
function modifyBabelLoaderRules (rules, modifier = r => r) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored the logic to make this painless and D.R.Y.

// Instrument for code coverage in development mode
const REACT_APP_ENV = process.env.REACT_APP_ENV ?? process.env.NODE_ENV ?? 'production'
if (REACT_APP_ENV === 'test') {
modifyBabelLoaderRule(config.module.rules)
config.module.rules = modifyBabelLoaderRuleForTest(config.module.rules)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

explicit assignment instead of in place modification on pass by reference.

return peerLocResolver.findLocations(peers, getIpfs)
},
getPromise: ({ store }) => peerLocResolver.findLocations(
store.selectAvailableGatewayUrl(), store.selectPeers()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we can just pull the list the gateway urls active and pass the peers.

latency: 'n/a',
addr: {
stringTuples: () => [[4, '123.123.123.123']]
selectPeers: () => [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reformatted for readability. tab +1

@@ -15,7 +15,7 @@ import AddConnection from './AddConnection/AddConnection'
import CliTutorMode from '../components/cli-tutor-mode/CliTutorMode'
import { cliCmdKeys, cliCommandList } from '../bundles/files/consts'

const PeersPage = ({ t, toursEnabled, handleJoyrideCallback, isCliTutorModeEnabled }) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused assignment.

@whizzzkid whizzzkid marked this pull request as ready for review November 9, 2022 18:43
@whizzzkid whizzzkid requested a review from a team as a code owner November 9, 2022 18:43
@whizzzkid whizzzkid requested review from SgtPooki and lidel November 9, 2022 18:43
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @whizzzkid

Tested Peers screen locally and works as expected (cachable raw blocks are loaded from gateway, and we no longer use POST to /api/v0 for fetching geoip data).

I am merging this in effort to include this and webtransport fix from #2057 in Kubo 0.17-rc1 (ipfs/kubo#9319)

@lidel lidel dismissed SgtPooki’s stale review November 9, 2022 22:21

Unblocked, concern is tracked in ipfs/aegir#1104

@lidel lidel merged commit 546fb5a into ipfs:main Nov 9, 2022
ipfs-gui-bot pushed a commit that referenced this pull request Nov 9, 2022
## [2.20.0](v2.19.0...v2.20.0) (2022-11-09)

 CID `bafybeibjbq3tmmy7wuihhhwvbladjsd3gx3kfjepxzkq6wylik6wc3whzy`

 ---

### Features

* add success notification on "set pinning" action ([#2038](#2038)) ([e410164](e410164))
* track remote pins in progress ([#1919](#1919)) ([d3a6524](d3a6524))
* update to ipfs-geoip v9 ([#2061](#2061)) ([546fb5a](546fb5a))

### Bug Fixes

*  /webtransport breaks status page ([#2057](#2057)) ([ea89e7f](ea89e7f)), closes [#2033](#2033)

### Trivial Changes

* pull new translations ([#2049](#2049)) ([f6062b2](f6062b2))
* pull new translations ([#2056](#2056)) ([e13ff17](e13ff17))
* pull new translations ([#2059](#2059)) ([0bb5bf3](0bb5bf3))
@ipfs-gui-bot
Copy link
Collaborator

🎉 This PR is included in version 2.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Peers screen to use ipfs-geoip v9.0.0
4 participants