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

feat(desktop): support local react devtools #1084

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

Matrixbirds
Copy link
Contributor

@Matrixbirds Matrixbirds commented Nov 11, 2021

The older React Developer Tools not compatible with react framework. It block my dev experience.
As many research, I found the root cause. Suggest to lock the React Developer Tools to avoid the weird error case.
Resolution: add third_party to lockReact Developer Tools version.

hyrious
hyrious previously approved these changes Nov 11, 2021
@BlackHole1 BlackHole1 merged commit 0ea1e5c into netless-io:main Nov 16, 2021
@LitoMore
Copy link
Member

As many research, I found the root cause. @Matrixbirds

What it is?

@BlackHole1
Copy link
Collaborator

As many research, I found the root cause. @Matrixbirds

What it is?

see: MarshallOfSound/electron-devtools-installer#205

@BlackHole1
Copy link
Collaborator

BlackHole1 commented Nov 17, 2021

Let me answer for him.

If your computer has developed other electron projects before, they also used electron-devtools-installer to load the react devtools, but their react devtools are very old and cause flat to report an error on startup. This is because the version of react-refresh we are using is not compatible with older versions of react devtools


So we discussed and decided to maintain the react devtools in a separate flat project.

@LitoMore
Copy link
Member

Can we use a forked package instead of storing those files to code base?

@BlackHole1
Copy link
Collaborator

Let me answer for him.

If your computer has developed other electron projects before, they also used electron-devtools-installer to load the react devtools, but their react devtools are very old and cause flat to report an error on startup. This is because the version of react-refresh we are using is not compatible with older versions of react devtools

So we discussed and decided to maintain the react devtools in a separate flat project.

The benefits of this are

  1. no need to worry about the installation of react devtools failing in China (network problems)
  2. no need to worry about conflicts with other projects' react-devtools
  3. Instead of waiting for electron-devtools-installer to fix it, or electron to fix it, we can always patch it. For more information on this see: [bug]: run dekstop failed on windows system #1031 (comment)

@BlackHole1
Copy link
Collaborator

Can we use a forked package instead of storing those files to code base?

This is difficult, I'm afraid, because the files are actually obtained from the crx decompression.
see: https://github.com/MarshallOfSound/electron-devtools-installer/blob/65ffa894bf635745d9399e79fb38682ccfd9c6e0/src/downloadChromeExtension.ts#L26-L32

@LitoMore
Copy link
Member

LitoMore commented Nov 17, 2021

no need to worry about the installation of react devtools failing in China (network problems)

This should not be called benefit. Developers should be able to solve network issues. We don't have to care network issue for them. Like #1037 does.

It is not elegant to store these unreadable files directly in the project. I think we can use git submodule or store these files in a new npm package.

The main point is: we don't need to track these files in the current repo.

We can use npm or GitHub repo (prefered) for storing this:

What is netless/electron-devtools: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#github-urls

"devDependencies": {
+	"electron-devtools": "netless/electron-devtools#0.0.1"
}

In desktop/main-app/src/window-manager/window-main/index.ts:

const extPath = path.resolve(
	__dirname,
	"..",
	"..",
	"..",
	"..",
	"..",
-	"third_party",
+	"node_modules",
+	"electron-devtools",

@BlackHole1
Copy link
Collaborator

you've got a point. I'm not going to use the git submodule approach to do this, as it's a bit of a pain to manage.

npm is a good idea

@Cyberhan123
Copy link
Contributor

Cyberhan123 commented Nov 17, 2021

you've got a point. I'm not going to use the git submodule approach to do this, as it's a bit of a pain to manage.

npm is a good idea

@LitoMore
yes, git submodule not a good idea in many condition

  1. You must use git submodule update before git commit -a
  2. The git submodule update not in right branch (‘detached HEAD’ state) so ,must check out branch into master or anyothers
  3. Not friendly for github or gitlab to manage PR

@LitoMore
Copy link
Member

LitoMore commented Nov 17, 2021

The GitHub repo what I mean is used for the npm package. It's easier for bumping versions and we don't have to publish any package to npm. I guess you guys misunderstood what I am talking about.

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

5 participants