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 CRA 2 and Storybook 5 #987

Merged
merged 9 commits into from
Mar 19, 2019
Merged

feat: update to CRA 2 and Storybook 5 #987

merged 9 commits into from
Mar 19, 2019

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Mar 11, 2019

Updates to Create React App 2 and Storybook 5.

Closes #888 and #986.

@fsdiogo fsdiogo self-assigned this Mar 11, 2019
@ghost ghost added the status/in-progress In progress label Mar 11, 2019
@fsdiogo fsdiogo requested a review from olizilla March 11, 2019 17:56
Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

This is great. The open question is what to do about linting? Should we go all in on prettier + eslint?

  • We should probably still have a lint command, but that could run eslint as it's already available.
  • We should document that this project uses prettier, and encourage people to use an editor plugin for it. Having it format on save is pretty helpful.
  • Should we run prettier before commit? I find any pre-commit hook quite annoying, so maybe not. But then we should double down on having folks run prettier in their editors. Same goes for eslint warning.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Mar 12, 2019

We should probably still have a lint command, but that could run eslint as it's already available.

💯 agree.

We should document that this project uses prettier, and encourage people to use an editor plugin for it. Having it format on save is pretty helpful.

I'd prefer to unburden the developers and make prettier optional.

Should we run prettier before commit? I find any pre-commit hook quite annoying, so maybe not. But then we should double down on having folks run prettier in their editors. Same goes for eslint warning.

True, maybe we should remove prettier altogether and just use eslint.

Quick and easy, what do you think?

@fsdiogo fsdiogo changed the title feat: update to CRA v2 feat: update to CRA 2 and Storybook 5 Mar 12, 2019
@fsdiogo fsdiogo requested a review from olizilla March 12, 2019 16:59
@olizilla
Copy link
Member

@fsdiogo can ya update from master pls

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Mar 18, 2019

Synced with master @olizilla.

@olizilla
Copy link
Member

package-lock got funky.

I can't start the app in local dev mode from an npm ci. am investigate.

./node_modules/create-hmac/browser.js
Module not found: Can't resolve 'create-hash/md5' in '/Users/oli/Code/ipfs-shipyard/ipfs-webui/node_modules/create-hmac'

@ghost ghost assigned olizilla Mar 19, 2019
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Mar 19, 2019

I used npm-merge-driver to automatically solve the package-lock.json conflicts.

You are right, doing npm ci is breaking the dev mode.

@olizilla
Copy link
Member

@fsdiogo you did the right thing, but it did not do the right thing! Is working now.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Mar 19, 2019

Nice! Want to shed some light of how you solved it please?

@olizilla
Copy link
Member

@fsdiogo The error showed that create-hmac depends on create-hash/md5 but looking at the files in my local node_modules/create-hash showed no md5.js which explained the error... looking at the other files in there it turned out that the entire dep was the wrong thing. create-hash was being resolved to a totally different dep:es-to-primitive... I did this:

$ cat node_modules/create-hash/package.json 
{
    "name": "es-to-primitive",
    "version": "1.2.0",
    "author": "Jordan Harband",
    "description": "ECMAScript "ToPrimitive" algorithm. Provides ES5 and ES2015 versions.",

so then i was all like whuuuuuuut!? so i checked the package-lock.json and found this weird nugget

"create-hash": {
     "version": "1.2.0",
     "resolved": "https://registry.npmjs.org/es-to-primitive/-/es-to-primitive-1.2.0.tgz",

sooo, that's weird, but it at least explains why npm was dropping es-to-primitve in where create-hash should be.

the solution, well, no magic here...

$ rm -rf node_module package-lock.json
$ npm i

🎉

...previously we had to be careful to preserve our package-lock.json when we were depending on an older version of ipld and a specific arrangement of ipld deps, but since we upgraded that, we're safe to re-build the package-lock when things get weird.

@olizilla olizilla merged commit 508e58c into master Mar 19, 2019
@olizilla olizilla deleted the feat/update-to-cra2 branch March 19, 2019 11:00
@ghost ghost removed the status/in-progress In progress label Mar 19, 2019
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Mar 19, 2019

betterthan

Thanks @olizilla!

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.

None yet

2 participants