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

Breaks up dripsy into @dripsy/core and dripsy #74

Merged
merged 9 commits into from
Mar 28, 2021

Conversation

cmaycumber
Copy link
Contributor

This pull request modularizes the dripsy monorepo into @dripsy/core and dripsy. Doing this will help us add packages in the future that rely upon additional dependencies.

This PR also has changes that reduce the bundle size by not including the entire theme-ui package which wasn't being used and now relies solely on @theme-ui/core.

@nandorojo
Copy link
Owner

I think this looks good to me. Will publish it to be sure.

@cmaycumber cmaycumber mentioned this pull request Feb 22, 2021
7 tasks
@nandorojo
Copy link
Owner

I think this is all good. One thing: could you remove peer dependencies from core? I think we should move these to dev dependencies now, especially with NPM's recent changes.

@nandorojo
Copy link
Owner

yarn add dripsy@mono uses this in 1.4.18

@cmaycumber
Copy link
Contributor Author

I think this is all good. One thing: could you remove peer dependencies from core? I think we should move these to dev dependencies now, especially with NPM's recent changes.

Yes for sure!

Curious about the NPM changes. I haven't caught wind of them yet. What impact do they have?

@nandorojo
Copy link
Owner

I saw Brent from expo tweeting about some major behavior changes of them auto installing or something like that.

Also, in general, I think they throw unnecessary warnings in the console too. Everyone using Dripsy has React Native installed, no need to bug them about it.

@cmaycumber
Copy link
Contributor Author

Cool, totally agree. Thanks for the insight.

@nandorojo
Copy link
Owner

No prob. Lmk if you're able to test that publish in your app smoothly

@nandorojo
Copy link
Owner

I think it's working well for me, will continue to test tomorrow and publish if it works for you!

@cmaycumber
Copy link
Contributor Author

Everything looks good on my end.

@nandorojo
Copy link
Owner

Sorry for the delay here, been really busy but looking forward to getting it merged shortly.

@nandorojo
Copy link
Owner

Let me know when you get the change to remove the peer deps and we'll get this out

@cmaycumber
Copy link
Contributor Author

Let me know when you get the change to remove the peer deps and we'll get this out

Sweet should be able to do this pretty soon.

"@theme-ui/css": "^0.4.0-rc.1",
"hoist-non-react-statics": "^3.3.2",
"theme-ui": "^0.4.0-rc.3"
"@dripsy/core": "*"
Copy link
Owner

Choose a reason for hiding this comment

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

@cmaycumber what's best practice for this? I'm unaware. Does the * just allow any version? Presumably we'd want the versions to match up. I saw lerna has an exact option, not sure if that's what we want or not.

Copy link
Owner

@nandorojo nandorojo Mar 1, 2021

Choose a reason for hiding this comment

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

Possibly related: lerna/lerna#359

Copy link
Owner

Choose a reason for hiding this comment

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

Since lerna publish will make all the version increment, I wonder if there's a way to make them also increment in the respective package.json files.

Copy link
Owner

Choose a reason for hiding this comment

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

I notice here that they use exact versions: https://github.com/system-ui/theme-ui/blob/ca00080aa3ceee704ad1fc4de99ada044e9b9193/packages/theme-ui/package.json#L22

I'm also wondering what the answer is for moti's sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmaycumber what's best practice for this? I'm unaware. Does the * just allow any version? Presumably we'd want the versions to match up. I saw lerna has an exact option, not sure if that's what we want or not.

Yeah, it's just the latest. I've looked around for something that manages exact versions in a monorepo and haven't been able to find anything that locks versions/updates them across packages.

We can switch to the exact versions I'm curious if there's a "best practice" solution for incrementing the versions or if we'll just have to write a script.

Copy link
Owner

Choose a reason for hiding this comment

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

So it looks like lerna does this for you if you use semver versioning ^0.x.y. I added versions like that in Moti, and when I ran lerna publish, it incremented all the dependencies for me.

@cmaycumber
Copy link
Contributor Author

Does this look gtg? I'm going to try to knock out some of the issues this weekend, specifically #73 , #5 , #71, and #57.

@nandorojo
Copy link
Owner

yup! last thing i'd say is to add the version for the internal dripsy dependencies so that lerna can handle incrementing those.

@cmaycumber
Copy link
Contributor Author

cmaycumber commented Mar 12, 2021

Got it. I'll add that in. So does Lerna automatically bump the dependencies?

Edit: Nvm read the comment above.

@nandorojo nandorojo mentioned this pull request Mar 12, 2021
@nandorojo
Copy link
Owner

I think it might be because I added this to Moti.

@cmaycumber
Copy link
Contributor Author

Let me know if the ch ages to the lerna file look correct.

@nandorojo
Copy link
Owner

yarn add dripsy@canary has this version: 1.4.19-alpha.324+c0ffc8e

@nandorojo
Copy link
Owner

nandorojo commented Mar 28, 2021

For some reason, running lerna publish didn't increment the version in the dependencies. It could be due to a minor version bump.

@nandorojo
Copy link
Owner

Okay I got this in my app. The good news, seems like it gets rid of theme-ui dependency. I have to refactor my many theme-ui imports (mostly types). I'll do that and see if the rest works.

@nandorojo
Copy link
Owner

nandorojo commented Mar 28, 2021

We'll need to add release notes for this (call it 1.5.x) specifying the need to replace any imports from theme-ui.

@nandorojo nandorojo merged commit 0aecb0b into nandorojo:master Mar 28, 2021
@nandorojo
Copy link
Owner

This took me a bit to complete since it required refactoring 100+ files in my real app while we're shipping many features. But finally, it's merged! Published with v1.5.2

@cmaycumber
Copy link
Contributor Author

This took me a bit to complete since it required refactoring 100+ files in my real app while we're shipping many features. But finally, it's merged! Published with v1.5.2

All good! Thanks for this.

I'll try to rip out some of my other PR's.

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.

2 participants