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.

Sign up for GitHub

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

Jump to bottom

wip: use connect core in suite-web #14959

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

wip: use connect core in suite-web #14959

wants to merge 1 commit into from

Conversation

karliatto
Copy link
Member

Description

Related Issue

Resolve

Screenshots:

@karliatto karliatto force-pushed the feat/use-core-in-suite-web branch 4 times, most recently from ab19949 to 60d634f Compare October 21, 2024 13:54
@mroz22
Copy link
Contributor

mroz22 commented Oct 23, 2024

nice, I can run
yarn suite:dev without building iframe, connect loads, there is no iframe injected
image

but:
1] I am getting some error, might be unrelated, rebase might be needed. Or maybe the reason is missing static files? If yes, it probably should be a sidequest to handle this error case better
2] Codewise it feels like we should simplify it a bit, there is lot of code duplication with https://github.com/trezor/trezor-suite/blob/feat/use-core-in-suite-web/packages/connect/src/index.ts#L0-L1. My original idea was to defined connect/src/impl/core-in-module, and then use it A] in connect, B] in connect-web/src/impl/core-in-module that would define needed overrides for web. is that possible?

@karliatto karliatto mentioned this pull request Oct 25, 2024
Closed
@karliatto
Copy link
Member Author

nice, I can run
yarn suite:dev without building iframe, connect loads, there is no iframe injected

Right, if you build iframe the whole discovery is it going to work, but not because it is using iframe but becuase currently I made some changes to iframe build so it works this way. So my goal now it go change it so it can be built separately but without much code duplication.

My original idea was to defined connect/src/impl/core-in-module

Here 88cd2c9 I made the changes to move core-in-module to connect/src/impl. But as you said probably big part of it could be reused from connect/src/index.ts`.

mroz22
mroz22 reviewed Oct 31, 2024
View reviewed changes
Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

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

I gave it a quick walkthrough and noted couple of questions.

packages/connect-iframe/package.json Show resolved Hide resolved
packages/connect-iframe/webpack/utils.ts Outdated
Comment on lines 8 to 11
case 'iframe':
return path.join(basePath, 'connect-iframe', 'build');
case 'suite-web':
return path.join(basePath, 'suite-web', 'build', 'static', 'connect');
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is to keep the same connect-related folder structure for web, right? the only change is that iframe is going away but the code that was previously in the iframe is still expecting some statics at the same paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is for using the same webpack configs for both projects.
So it is built directly from suite-build instead of copying the build from connect-iframe as was done before.

packages/connect-web/src/module/index.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks nice, core-in-module for web, reusing coreInModule from node only with some overrides 👍

packages/connect/src/data/DataManager.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand what is going on here. if iframe then load locally? (I didn't check internals of httpRequest yet. anyway, this is something I will be thinking how to avoid bringing this kind of decision making logic into DataManager. At the first glance it doesn't feel right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that feel wrong to me as well, there is other way to do it like 7696f94#diff-54a3dc9015ab9933751016bc35e7987f834b59b24a8d0cc05ef51d1ddabdd1c0 and it works same.

packages/connect/src/impl/core-in-module.ts
Comment on lines +51 to +55
const { initCoreState, initTransport } = await import(
/* webpackIgnore: true */ `${connectSrc}js/core.js`
).catch(_err => {
this._log.error('_err', _err);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for dynamic load here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Making it flexible, so in the context of suite-web connectSrc is going to be different as if we use it somewhere else.
How would you do it?

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024 •
edited
Loading

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 10
  • More info

Learn more about 𝝠 Expo Github Action

@karliatto karliatto force-pushed the feat/use-core-in-suite-web branch 3 times, most recently from 4a4efc6 to 2ba4341 Compare November 7, 2024 08:26
@karliatto karliatto force-pushed the feat/use-core-in-suite-web branch 3 times, most recently from 62609c5 to f437505 Compare November 7, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroz22 mroz22 mroz22 left review comments

@komret komret Awaiting requested review from komret komret will be requested when the pull request is marked ready for review komret is a code owner

@szymonlesisz szymonlesisz Awaiting requested review from szymonlesisz szymonlesisz will be requested when the pull request is marked ready for review szymonlesisz is a code owner

@martykan martykan Awaiting requested review from martykan martykan will be requested when the pull request is marked ready for review martykan is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karliatto @mroz22