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.

Sign up for GitHub

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

Jump to bottom

Migrate to yarn v2 #7575

Merged
merged 24 commits into from
Sep 27, 2021
Merged

Migrate to yarn v2 #7575

merged 24 commits into from
Sep 27, 2021

Conversation

clemsos
Copy link
Member

@clemsos clemsos commented Sep 22, 2021

Description

Let's use yarn v2 so we can use plugin for deployment and improve build/dev workflow.

From previous attempt > #7446

Issues

Refs #7427

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

@cla-bot cla-bot bot added the cla-signed label Sep 22, 2021
@julien51
Copy link
Member

Oh wow!

julien51
julien51 approved these changes Sep 22, 2021
View reviewed changes
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

This looks so simple I am almost worried we're missing something here.

@julien51
Copy link
Member

Feel free to merge in the morning @clemsos :)

@julien51
Copy link
Member

Please, make a final rebase before mergimg though!

@clemsos
Copy link
Member Author

clemsos commented Sep 23, 2021

This looks so simple I am almost worried we're missing something here.

yep there were some missing commits there...

@clemsos
Copy link
Member Author

clemsos commented Sep 23, 2021

seems that we are good to go now. The yarn install on local 10min to 10s so that feels much better while developing. On CI, we may have lost some speed but needs to be assessed once docker cache can be reused. Also we have a nice generic solution for prod builds

@clemsos clemsos mentioned this pull request Sep 23, 2021
Merged
6 tasks
@julien51
Copy link
Member

🚢 it!

julien51
julien51 approved these changes Sep 23, 2021
View reviewed changes
locksmith/package.json Show resolved Hide resolved
@julien51
Copy link
Member

That latest error

TypeError: createKeccakHash is not a function

      82 |   signData(data) {
      83 |     const privateKey = toBuffer(this.wallet.privateKey)
    > 84 |     const sig = sigUtil.signTypedData(privateKey, { data })

Does not make much sense to me!

@clemsos
Copy link
Member Author

clemsos commented Sep 24, 2021 •
edited
Loading

It's an issue with jest transpiling ts from node modules package. So basically some tweak in jest conf to find :/

julien51
julien51 reviewed Sep 26, 2021
View reviewed changes
unlock-app/jest.config.js Outdated
},
coveragePathIgnorePatterns: ['/node_modules/', 'src/stories/.*/*.stories.js'],
transformIgnorePatterns: ['[/\\\\]node_modules[/\\\\].+\\.(js|jsx|ts|tsx)$'],
transformIgnorePatterns: [
"/node_modules/(?!ethereum-cryptography)",
Copy link
Member

Choose a reason for hiding this comment

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

Oh maybe that's because of this??

Copy link
Member Author

Choose a reason for hiding this comment

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

yep its related. I had to add it because the module needs to be transpiled but now it doesnt find keccak (which uses node-gyp so maybe this is it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

it was some missing <rootDir> in jest config 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm there was more to it. Babel's name ttly make sense now. A real punishment!

Copy link
Member

Choose a reason for hiding this comment

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

Oh dang! Let me review once more!

@clemsos
Copy link
Member Author

clemsos commented Sep 27, 2021

arf CircleCI is down...

@julien51
Copy link
Member

Wooot!!!

julien51
julien51 approved these changes Sep 27, 2021
View reviewed changes
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Ok, I think I have a decent understanding of everything that's going on. Let's ship it!

unlock-app/jest.config.js
},
coveragePathIgnorePatterns: ['/node_modules/', 'src/stories/.*/*.stories.js'],
transformIgnorePatterns: [
"/node_modules/(?!ethereum-cryptography)",
'[/\\\\]node_modules[/\\\\](?!(ethereum-cryptography)).+\\.(js|jsx|ts|tsx)$',
Copy link
Member

Choose a reason for hiding this comment

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

Looks a bit scary but let's hope we can at least refactor on used a shared module for all this soon!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I kept the original regex... scary indeed

.dockerignore
.vscode

# Elastic Beanstalk Files
Copy link
Member

Choose a reason for hiding this comment

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

Can't wait to get rid of all this!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes all these aws files in locksmith can be deleted too

@clemsos clemsos merged commit 8a3bb0e into master Sep 27, 2021
@clemsos clemsos deleted the yarn-v2 branch September 27, 2021 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien51 julien51 julien51 approved these changes

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@clemsos @julien51