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

Update CLI with latest main #555

Merged
merged 23 commits into from
Oct 5, 2023
Merged

Update CLI with latest main #555

merged 23 commits into from
Oct 5, 2023

Conversation

carletex
Copy link
Member

@carletex carletex commented Oct 3, 2023

main Back-merge from 02dd022 to b33a87d. Check commits.

I created a cli-backmerge branch from cli, where I merged main and fixed conflicts. That way we can create this PR and check & test the PR before merging to CLI. Maybe we could add this to DEV_GUIDE md file

Note: Until main === cli, let's do this (back-merge main) once a week. If not, it gets messy.

@technophile-04
Copy link
Collaborator

Was trying to test it and when I do yarn next:check-types I get bunch of react types error :
Screenshot 2023-10-03 at 7 43 20 PM

also yarn next:lint doesn't give me any error so might be something to do with TS version or types/react, will try to debug 🙌

Steps to reproduce => select hardhat as option and choosed yes for installation

@technophile-04
Copy link
Collaborator

technophile-04 commented Oct 3, 2023

Curious about why this happened, I did some digging, and here's my finding :

SE-2 main branch yarn.lock has this :
Screenshot 2023-10-03 at 8 26 01 PM

When you create a new template from this PR, its yarn.lock contains this :
Screenshot 2023-10-03 at 8 30 34 PM

notice that it create two blocks one with version 18.2.24 and other with 18.0.38 is this what causing the problem ?

I also tried removing "~" from ~18.0.21 package.json and regenrating the app but still it makes two block

is that some other package is dependent on latest version of @types/react because of which it is generating two blocks? I might be wrong here and lol I have a knowledge gap on how this works

is this related somewhere to @rin-st comments => #541 (comment) (might be unrealted)


For now, the solution seems #555 (comment) but just curious why this happened and this affects us in future

@carletex
Copy link
Member Author

carletex commented Oct 4, 2023

Thanks for the investigation @technophile-04 !

Some more stuff that I found (yarn why is great):

This is the CLI:
image

This is main:
image

In CLI we have 2 diff versions because:

  • newer version of the VERCEL package (28.15.1 => 28.15.7)
  • newer version of types/react-copy-to-clipboard (5.0.4 => 5.0.5)

These two use a newer version of types/react (18.2.24).


What I did then is to remove the ~ in the package.json file (only for next), so it installs the same version as in the lockfile (at least the top dependency!):

image

So, it the vercel dep is gone, but not the clipboard one. I think this is exactly what @rin-st was talking about here: #541 (comment)

Even if we install the same top dep (5.0.4) it uses a newer version of @types/react. Why? Because package.json file of @types/react-copy-to-clipboard:

    "dependencies": {
        "@types/react": "*"
    },

😢


So not having a lockfile is tricky.

We can remove ~, but things can always break in the future because a new release of sub-dependency updates and breaks something.

A decent solution:

  • Remove ~
  • Have some kind of automated testing that runs every day, installs SE-2 with different options, runs some commands, and checks errors. That way we can identify issues early.

Not ideal, but better than nothing.

Thoughts?

--

Also check this comment: #555 (comment)

This could help for errors related to @types packages

@technophile-04
Copy link
Collaborator

technophile-04 commented Oct 4, 2023

Tysm @carletex !! Just really awesome explanation love it!!, almost all my doubts are cleared nicely and now it makes complete sense too!! 🙌

So not having a lockfile is tricky.

We can remove ~, but things can always break in the future because a new release of sub-dependency updates and breaks something.

A decent solution:

  • Remove ~
  • Have some kind of automated testing that runs every day, installs SE-2 with different options, runs some commands, and checks errors. That way we can identify issues early.

Not ideal, but better than nothing.

Yes, I agree 💯

Just very last one doubt :

Even If we remove ~ ( i.e we will be using fixed verion) and which might again cause the issue that we discussed and mentioned in #541 (comment).

For the above reason, we are planning to set tests but won't there be a case lets suppose :

We are using library A which has internal dependency as sum : * (which is expected to return sum of given array )

Now lets suppose there is another library we are using B which has an internal dependency sum : latest (since this is latest version API has changed and instead it returns object with sum inside eg. {sum : 54} )

This will affect A right ? and A internal working will break right ?

It will be really hard for us to debug too right ? Since A might using sum very deep down which causes some of A api's break which we are using in SE-2 also this will be hard to catch in automated tests?

If we use ^ everywhere instead of removing ~, will this make our debugging task easy like there will be errors but these errors will be more closer to us instead in previous case where there was error deep down because of wrong internal dependency used

Just trying to make the point that debugging will get a lot harder if the wrong internal dependencies are installed, but really don't know If I am making sense here (and might be wrong) and this might not even happen

Really sorry for overthinking this too much please feel free to ignore the above since its kinda made up the situation in my head 😅 but yeah even I feel removing ~ and setting test is a nice solution and completely up for it 🙌

@carletex
Copy link
Member Author

carletex commented Oct 4, 2023

@technophile-04 I think I understand what you are saying.

doing sum : latest is not a very good practice for any lib (but it seems common for @types libs). But yeah, the risk is there

It will be really hard for us to debug too right ? Since A might using sum very deep down which causes some of A api's break which we are using in SE-2 also this will be hard to catch in automated tests?

If we have good automated tests, I guess we could detect that something is not working (deploy, start, checking types, etc), right? And the we can investigate the root of it and fix it (it'll prolly be a versions that needs to be updated)

I think I'm gonna update @types to ^ for now.

Also btw I think we should make a quick another patch PR cli because npx create-eth@latest is broken now because of this

Maybe we can merge this one? I'll send the update in a bit.

@carletex
Copy link
Member Author

carletex commented Oct 4, 2023

This is working for me @technophile-04

If it works for you.. let's merge and publish!

@rin-st
Copy link
Collaborator

rin-st commented Oct 4, 2023

Great investigations! Experimented with it too but didn't find something better

I think this is exactly what @rin-st was talking about here

I thought: child dep breaks our dep -> our dep breaks our app. But it's even worse. child dep breaks our app 🤷‍♂️ . I didn't know about that problem before and thought package managers can manage it and use different versions needed for our dep and child dep. But it's not always true.

Also I thought "*" takes already installed libs with priority to our deps. Again it's not true. "lib": "*" is almost equal "lib": "latest".

I think I'm gonna update types to ^ for now.

It works for me, but I believe we also need to think later about removing that "*" deps. Because it will break cli after every major update of @types/react and ^ for types will not help. It seems not so difficult to change that react-copy-to-clipboard. For example there is a hook in usehooks-ts (I think we need to move from usehooks-ts too, but it's another story).

Have some kind of automated testing that runs every day, installs SE-2 with different options, runs some commands, and checks errors. That way we can identify issues early.

Not sure how to do it, but if it's possible why not.

@technophile-04 sorry I don't know the answer to your question. Will try to read it again tomorrow 😄

Thanks!

ps. FYI, if you didn't see it yet facebook/react#24304 (comment)

@technophile-04
Copy link
Collaborator

doing sum : latest is not a very good practice for any lib (but it seems common for https://github.com/types libs).

Yup yup 💯💯 I rarely see lib using latest or * for their dependencies and yes it seem common with @types which makes sense

Tysm for answering 🙌

@technophile-04 sorry I don't know the answer to your question. Will try to read it again tomorrow 😄

No worries Rinat 😆 Just ignore it trust me it was very highly kind of unreal situation cooked up in my head and lol just dumped it in that comment, but yeah as Carlos mentioned libraries mentioning latest or * for their internal dependencies is very rare


I tested it and works nicely also tried doign yarn vercel and it worked nicely => https://test-cli-h.vercel.app

Just pushed a small tweak at foundry at 4f6fcb5 which we did at #531

and added chandeset 🙌

Tysm guys merging this!

@technophile-04 technophile-04 merged commit 9c967d9 into cli Oct 5, 2023
@technophile-04 technophile-04 deleted the cli-backmerge branch October 5, 2023 04:52
@rin-st
Copy link
Collaborator

rin-st commented Oct 5, 2023

We are using library A which has internal dependency as sum : * (which is expected to return sum of given array )
Now lets suppose there is another library we are using B which has an internal dependency sum : latest (since this is latest version API has changed and instead it returns object with sum inside eg. {sum : 54} )
This will affect A right ? and A internal working will break right ?

sum: * is the same as sum: latest. So sum from A will break A. I hope no one does "*" or latest for non-type deps.

If you meant by * some exact version as I understand it depends. Let's say dep in A has version ^1.0.0 and latest version is 1.1.0. Package manager will resolve it for one version 1.1.0 since it fits both versions. But if it's not resolvable like ^1.0.0 and 2.0.0 package manager creates two versions or sometimes create one and show warnings about wrong versions in console.

In our case it creates two versions. The question for me how and why types from react-copy-to-clipboard seen and used at the top level

It will be really hard for us to debug too right ?

Not sure. I think mostly same like other error. See stack trace -> find what's wrong

@carletex
Copy link
Member Author

carletex commented Oct 5, 2023

Thank y'all for this discussion. Learning a lot!

I think we should have this discussion open (maybe a new issue?) because this is something that we are going to face. I guess the base issue is not having a lockfile (which we could use in our advantage to allow people to use their desired package manager, as Shiv mentioned.

Something that we could explore in some cases => yarn resolutions (so we can set the version of any sub-dependency):

"resolutions": {
   "@types/react": "18.2.21"
}

@technophile-04
Copy link
Collaborator

technophile-04 commented Oct 5, 2023

I hope no one does "*" or latest for non-type deps.
Let's say dep in A has version ^1.0.0 and latest version is 1.1.0. Package manager will resolve it for one version 1.1.0 since it fits both versions. But if it's not resolvable like ^1.0.0 and 2.0.0 package manager creates two versions or sometimes create one and show warnings about wrong versions in console.

Yup 💯💯, Tysm Rinat 🙌

After reading alot of issues/discussions :

Also I thought "" takes already installed libs with priority to our deps. Again it's not true. "lib": "" is almost equal "lib": "latest".

Yeah "*" considers newest version but only in case of yarn checkout this => yarnpkg/yarn#4489 (comment)

If we would have used npm or pnpm then it uses our dependency as mentioned here in this table => Shopify/ui-extensions#324 (comment)

lol @types/reacterror seems pretty common when you don't have .lock file present.

Also this @types/react error happes only when there are multiple versions of @types/react as mentioned here => software-mansion/react-native-svg#1741 (comment)

Looking at solutions, people seem rely on yarn resolution for this and we can do something like this :

"resolutions": {
   "@types/react": "18.2.25"
}

I think we are good using ^ for now since in Carlos comment first image there is one @remix/.. which uses ^ and if you do ^ latest minor version is used and we will still have only one @types/react version

Because it will break cli after every major update of @types/react and ^ for types will not help

I think there were will long way before major version of @types/react is released 😅 because @types/packageName will only release Major version if packageName releases major version as mentioned here, and lol React V19 seems long way from now but yeah this will be the case

But maybe by then we give people option to select different package manager like pnpm or bun 🙌

@rin-st
Copy link
Collaborator

rin-st commented Oct 5, 2023

I think we should have this discussion open (maybe a new issue?) because this is something that we are going to face

It would be good, with sum up of this pr discussion, or just with link to it

I think we are good using ^ for now since in Carlos comment first image there is one @remix/.. which uses ^ and if you do ^ latest minor version is used and we will still have only one @types/react version

Agree

I think there were will long way before major version of @types/react is released 😅 because @types/packageName will only release Major version if packageName releases major version as mentioned here, and lol React V19 seems long way from now but yeah this will be the case

😄 yes, but we can forget about this issue when it will happen. And I'm afraid it can happen not only for react, but for smaller packages too. Hope I'm wrong

If we would have used npm or pnpm then it uses our dependency

Thanks! Chatgpt deceived me 😄 . It said that it works for every package manager.

But maybe by then we give people option to select different package manager like pnpm or bun

Tried to test it, pnpm resolves * and takes 18.0.21 so it works 🙌.
But bun behaves like yarn, so the same problem for now.

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

9 participants