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

chore: mergeback next -> identity-provider/2021-01-04 #216

Merged
merged 32 commits into from Feb 17, 2021

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Feb 16, 2021

Trying ways of landing #208

Why?

hansl and others added 14 commits February 16, 2021 11:06
For now we just re-parse it in BigNumber.
This includes a change to request_id from the identity-provider PR
(#132) that I thought was related to something else, but is essential
here with the new lerna packages.

This moves the CI to using lerna entirely, and prevents running
npm install (instead telling the user to run npx lerna bootstrap).
@gobengo
Copy link
Contributor Author

gobengo commented Feb 16, 2021

(Donno if we want lerna bootstrap to use hoist or not. But setting ci=true ensures that lerna won't try to write packages/*/package-lock.json

@gobengo
Copy link
Contributor Author

gobengo commented Feb 16, 2021

I am skeptical of the * version ranges between intra-monorepo dependencies in packages/*/package.json... https://github.com/wixplosives/sample-monorepo/blob/master/packages/app/package.json#L18

@gobengo
Copy link
Contributor Author

gobengo commented Feb 16, 2021

The npm ci scripts in agent,authentication try to run npm run build, but we should have those ci scripts expect build to have already been run by lerna run build in a dependency-tree-aware way. That way lerna can make sure that agent is built before authentication (otherwise it fails)

@hansl
Copy link
Contributor

hansl commented Feb 16, 2021

Dont' waste too much time on this. In 132 you never built the websites using lerna, so using a * wildcard in lerna uncovers a lot of issues. We can fix tomorrow.

@gobengo
Copy link
Contributor Author

gobengo commented Feb 16, 2021

so using a * wildcard in lerna uncovers a lot of issues. We can fix tomorrow.

Sounds like you're referring to lerna.json not packages/*/package.json

I am skeptical of the * version ranges between intra-monorepo dependencies in packages/*/package.json... https://github.com/wixplosives/sample-monorepo/blob/master/packages/app/package.json#L18

@gobengo
Copy link
Contributor Author

gobengo commented Feb 17, 2021

I am skeptical of the * version ranges between intra-monorepo dependencies in packages/*/package.json... https://github.com/wixplosives/sample-monorepo/blob/master/packages/app/package.json#L18

My skepticism was incorrect! Still feels odd to not have at least a major semver range in there (which I thought worked, but if this works too idc)

@gobengo gobengo marked this pull request as ready for review February 17, 2021 01:15
"packages/authentication"
"packages/authentication",
"packages/bootstrap",
"e2e/*"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notably this does not include other packages/*.

  1. Those can be added after this merge, which is still a valid mergeback (and then some!) of next -> feat(identity-provider): Identity Provider v1 #132
  2. next currently only has identity-provider on top of the ones explicitly enumerated here, and idp is brand new code we should be thrilled to just iterate on top of once feat(identity-provider): Identity Provider v1 #132 is merged

"@typescript-eslint/parser",
"eslint",
"eslint-plugin-jsdoc",
"typescript"
Copy link
Contributor Author

@gobengo gobengo Feb 17, 2021

Choose a reason for hiding this comment

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

These hoist devDependencies that lerna doesn't even try to install (lerna devs consider devDeps it out of scope), but that e.g. lerna run lint -> subpackage npm run lint will expect to be around, e.g. eslint.

Once this is merged, agent-js-devtools package can maybe completely go away?

Copy link
Contributor

Choose a reason for hiding this comment

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

it shoudln't be in next anymore.

…/agent-js into bengo/1613514044/132-mergeback-1
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