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: Removes Apps and Demos #329

Merged
merged 12 commits into from Apr 7, 2021
Merged

chore: Removes Apps and Demos #329

merged 12 commits into from Apr 7, 2021

Conversation

krpeacock
Copy link
Contributor

To focus the scope of agent-js, we are moving the identity provider into its own repo, which removes the need for IdentityProvider, Bootstrap, and our demos

@krpeacock krpeacock requested review from gobengo and hansl March 29, 2021 18:22
@hansl
Copy link
Contributor

hansl commented Mar 29, 2021

I'll block this for a week or two while we deprecate bootstrap, just in case there are issues where we need to republish it.

Also workers depend on those two apps.

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM just need to make sure we don't shoot ourselves in the foot. We still have two features incoming for the worker and bootstrap is still used as a fallback for ic0.app.

@krpeacock
Copy link
Contributor Author

Agreed, there is no urgency here

@krpeacock
Copy link
Contributor Author

Okay, I think that we're ready to move forward, if the code looks good.

Changes here are:

  • removes apps, workers, and demos (they have been relocated)
  • incorporates changes to replace lerna with npm workspaces and enable tests to be run directly by jest

@krpeacock krpeacock requested a review from hansl April 5, 2021 23:58
@krpeacock krpeacock requested a review from hansl April 7, 2021 16:46
"make:docs/reference": "",
"publish:release": "",
"test:coverage": "",
"test": "jest"
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a test script here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and I've removed "jest". However, I'll leave a blank script in case anyone wants to run tests using npm run test --workspaces.

I'll also open up an issue on the npm github about skipping empty scripts. There's no reason we should have to include blank scripts in workspaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM. Good cleanup. Love it!

@krpeacock krpeacock merged commit 22390b4 into next Apr 7, 2021
@krpeacock krpeacock deleted the kyle/remove-apps branch April 7, 2021 19:14
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