Skip to content
This repository has been archived by the owner on Aug 23, 2019. It is now read-only.

feat: swap quick-lru by hashlru #259

Merged
merged 1 commit into from
Jun 6, 2018
Merged

feat: swap quick-lru by hashlru #259

merged 1 commit into from
Jun 6, 2018

Conversation

hugomrdias
Copy link
Member

@hugomrdias hugomrdias commented May 29, 2018

This removes the only dependency using generators in the ipfs/libp2p ecosystem.
Next version of create-react-app will support ipfs out-of-box with this change.

hashlru is the original implementation sindre used for quicklru
https://github.com/sindresorhus/quick-lru

Inspired by the hashlru algorithm, but instead uses Map to support keys of any type, not just strings, and values can be undefined.

Browser bundling ecosystem is moving towards running babel preset env through the dependencies and stop relying on pre built bundles to optimize end user bundles. But babel preset env handles everything but generators, this change will make any package that uses js-libp2p-switch more compatible.

This removes the only dependency using generators in the ipfs/libp2p ecosystem.
Next version of create-react-app will support ipfs out-of-box with this change.
@olizilla
Copy link
Contributor

This is rad. I'm just re-testing this after getting caught up in #262 which i ran into when first testing this.

Just to clarify for others, the key point here is

This removes the only dependency using generators in the ipfs/libp2p ecosystem.
Next version of create-react-app will support ipfs out-of-box with this change.

We really want create-react-app to work out of the box with js-ipfs as it's a super popular app scaffolding tool and we get lots of bug reports when we don't. As I understand it, by switching out quick-lru for hashlru we remove the only dep in the js-ipfs tree that needs a generators implementation, and in turn that removes the last issue that would otherwise force a create-react-app user to have to eject and customise their config, which is something people like to avoid, so they can easily update to newer versions of create-react-app as they are released.

@hugomrdias can you add some notes here on what to check for if someone wants to test this change. I'm following the steps in ipfs/js-ipfs#1321 (comment)

I'm able to run yarn start with and without the resolutions change to use this PR, so is there something else I should be looking out for? I've cleared out the node_modules and re-installed when switching. I know you explained it to me already, but I could use a reminder!

@daviddias daviddias requested review from jacobheun and removed request for daviddias June 4, 2018 08:46
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

@hugomrdias what's the process for verifying this manually since there are no tests? I attempted to verify it from the linked comments, but I didn't encounter a build issue before adding resolutions to the package.json for his fix.

@hugomrdias
Copy link
Member Author

@jacobheun the current tests should be enough because this does not add interface/logic changes.

@olizilla and @jacobheun the resolutions is just to make sure yarn uses this branch to resolve deps. Did you guys tried the build command from create-react-app? because start always worked because it uses a dev config. or did you remove browserslist from package.json ?

Default browserslist config

"browserslist": {
    "development": [
      "last 2 chrome versions",
      "last 2 firefox versions",
      "last 2 edge versions"
    ],
    "production": [
      ">1%",
      "last 4 versions",
      "Firefox ESR",
      "not ie < 11"
    ]
  }

So to better explain the problem, a couple of points:

  • problem comes from running yarn build which uses browserslist.production that list includes browsers that don't support generators, babel will expect that babel/runtime/regenerator exists as a polyfill.
  • babel-preset-env doesn't add regenerator when resolving polyfills for the targets (ie browserslist), don't ask me why -_-
  • CRA (2.0) has 2 configs for babel, one for app source code and another for dependencies.
  • the app code config, adds regenerator using transform-runtime
  • the deps config does NOT, so we need this PR to remove the only ipfs dependency that uses generators.

@jacobheun
Copy link
Contributor

@hugomrdias thanks for the additional info. I was just looking for clarity around the manual verification process to ensure everything was working okay.

I was able to verify this by doing:

$ create-react-app .
$ # add ipfs into the app
$ yarn bulld # fails
$ yarn upgrade react-scripts@2.0.0-next.66cc7a90
$ yarn build # fails
$ # add resolutions per https://github.com/ipfs/js-ipfs/issues/1321#issuecomment-393112809
$ yarn build # succeeds

I will get this merged and released, thanks!

@jacobheun jacobheun merged commit 888e973 into master Jun 6, 2018
@ghost ghost removed the in progress label Jun 6, 2018
@jacobheun jacobheun deleted the feat/swap-quick-lru branch June 6, 2018 16:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants