-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: support for more assets #434
Conversation
alecdwm
commented
Jan 11, 2023
•
edited
Loading
edited
- Test that deploying the new squid won't break the existing live build (due to adding new tokens which the live build doesn't support)
- Deploy the new squid
… state query results
ad2a4cb
to
95820d0
Compare
1685a6f
to
3fae1b2
Compare
3fae1b2
to
f6c9f39
Compare
8dfec82
to
a327f27
Compare
it breaks the resolution of dependencies-of-dependencies in downstream projects e.g. for @talismn/web -> @talismn/balances-react -> @talismn/balances @talismn/balances resolves to the version with a git hash beginning with the lowest letter of the alphabet, rather than the most recently published
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments
Too complex to review in detail but let's do some testing
metadata.__kind === "V12" || | ||
metadata.__kind === "V13" | ||
) { | ||
// we can't parse metadata < v14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could put a link to some docs or a GH issue on this so future devs can understand why v14 is different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
I'll link to this pr. Before v14 the client had to know the chain types ahead of time, whereas with v14 it's now possible to fetch them from the chain RPC.
|
||
suppressPortableRegistryConsoleWarnings() | ||
|
||
const importDynamic = new Function("modulePath", "return import(modulePath)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this will be permissable in manifest v3. The guidelines refer to 'remote code', but I suspect that reviewers will just look at new Function
and say no.
https://developer.chrome.com/docs/extensions/mv3/mv3-migration-checklist/#api-remote-code
I can see this is a tricky issue, are there any other potential methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yikes :|
Maybe we can just write this module as plain javascript (then tsc won't convert the import
into a require
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue for it on the tsc github with some alternatives
microsoft/TypeScript#43329
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is worth trying
microsoft/TypeScript#43329 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inclusion
did not work (causes Critical dependency: the request of a dependency is an expression
errors in frontend bundles), now using a different approach (import a .cjs file which is not transpiled by tsc, use dynamic import with vite-ignore and webpackIgnore: true magic comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM