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

add Prettify type helper locally #541

Merged
merged 2 commits into from
Sep 25, 2023
Merged

add Prettify type helper locally #541

merged 2 commits into from
Sep 25, 2023

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Sep 24, 2023

Description

It seems that NextJs deployments fails since in latest version viem(1.12.x) they have moved Prettify type from "viem/dist/types/types/utils" => "viem/types/utils"

Now due to #419, our version viem exports from "dist" but when we upload to vercel it uses new viem where the location from which it is exported is changed

Reported BuidlGuidl/PWA-base#1 (comment) & #444 (comment)


As mentioned by Rinat #444 (comment) addingPrettify locally works and also it makes sense to localize this "helper type" since it is famous and we also shouldn't rely on Viem to export this for us because they might move to another place in future/change name/remove without mentioning it since its not their main api

@technophile-04 technophile-04 mentioned this pull request Sep 24, 2023
@carletex
Copy link
Member

Now due to #419, our version viem exports from "dist" but when we upload to vercel it uses new viem where the location from which it is exported is changed

Yeah, we really need to tackle this issue, where that deps local version != deployed version. #444 is a solution, but I'm not 100% sold on it. I'll comment soon.

The other option is locking the version in package.json but I feel you guys don't like that? I don't see any issues taking that approach. When we want to update we just update the version manually (which arguably makes us more conscious about updating / the version we are updating to), or even temporarily adding ^ before updating a given dependency and locking it later. Maybe not as convenient as just running yarn update on a given dependency... by it's definitely better than having a version mismatch between the local & prod env. Am I missing something?


Also for this PR: Shouldn't we just update viem (so it saves in the .lock file executed locally). It'll still fail for SE-2 forks/clones, but it'll be fixed on fresh installs.

Thanks!

@carletex
Copy link
Member

Also for this PR: Shouldn't we just update viem (so it saves in the .lock file executed locally). It'll still fail for SE-2 forks/clones, but it'll be fixed on fresh installs.

Answering myself here, since I didn't read this:

As mentioned by Rinat #444 (comment) addingPrettify locally works and also it makes sense to localize this "helper type" since it is famous and we also shouldn't rely on Viem to export this for us because they might move to another place in future/change name/remove without mentioning it since its not their main api

Make sense!

Merging this! but will still love to hear your thoughts here: #541 (comment)

@technophile-04 @rin-st @Pabl0cks

@carletex carletex merged commit 41b1671 into main Sep 25, 2023
1 check passed
@carletex carletex deleted the fix/prettify-type branch September 25, 2023 10:49
@technophile-04
Copy link
Collaborator Author

The other option is locking the version in package.json but I feel you guys don't like that? I don't see any issues taking that approach. When we want to update we just update the version manually (which arguably makes us more conscious about updating / the version we are updating to), or even temporarily adding ^ before updating a given dependency and locking it later. Maybe not as convenient as just running yarn update on a given dependency... by it's definitely better than having a version mismatch between the local & prod env. Am I missing something?

Doing some very basic research, I don't see any big drawbacks of removing "^" so I am completely up for it 🙌

but would love to know other thoughts if there are any big drawbacks that I might have missed also cc @damianmarti @sverps maybe too

@rin-st
Copy link
Member

rin-st commented Sep 25, 2023

The other option is locking the version in package.json but I feel you guys don't like that? I don't see any issues taking that approach

The main difference I see is that our dependencies (for example "our-dep": "1.0.0") have their package.json with their dependencies inside it (for ex "child-dep": "^2.0.0"), and that child deps will be different without package.lock. It's a small chance that one child dep will break our dep, but we have hundreds of child deps and because of that chances of breaking again someday are not too small.


regarding this pr fix, we should avoid importing like that, because this type is not even exported from viem

@damianmarti
Copy link
Member

I always prefer to have a fixed version of everything, I really don't like ^ too much because sometimes there are some breaking changes between minor versions (if the .lock is used, this is not an issue because the version is fixed there).

So, I don't see any issue with having a version fixed at the package.json (this library or whatever), and being explicit when we want to update it to another fixed version.

You guys are great! You are on all the little details! :-)

@carletex
Copy link
Member

Thanks @technophile-04 @rin-st @damianmarti for your view on this <3

I'll move the discussion to the pnpm PR, since If we go with fixed versions, we could rethink pnpm.

I'll prepare my allegations soon haha

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.

5 participants