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

fix(template-vite): ignore browser field for isomorphic packages #3218

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

jclab-joseph
Copy link
Contributor

@jclab-joseph jclab-joseph commented Apr 18, 2023

Packages like axios have a browser field in package.json. The main process needs to resolve the node module, not the browser module.

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Packages like axios have a browser field in package.json.
The main process needs to resolve the node module,
not the browser module.

@jclab-joseph jclab-joseph requested a review from a team as a code owner April 18, 2023 03:21
@BlackHole1
Copy link
Member

Request code review
/cc @caoxiemeihao

Copy link
Member

@caoxiemeihao caoxiemeihao left a comment

Choose a reason for hiding this comment

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

@BlackHole1
Copy link
Member

@jclab-joseph CI Lint failed. You can try execute:

npx prettier --write .

Packages like axios have a browser field in package.json.
The main process needs to resolve the node module,
not the browser module.
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM

@BlackHole1
Copy link
Member

@caoxiemeihao Do you have anything to add?

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 19, 2023

@caoxiemeihao Do you have anything to add?

Wait a moment 😅

Copy link
Member

@caoxiemeihao caoxiemeihao left a comment

Choose a reason for hiding this comment

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

Just keep the browserField, and in the future as well :)

export default defineConfig({
  resolve: {
    // Some libs that can run in both Web and Node.js, such as `axios`, we need to tell Vite to build them in Node.js.
    browserField: false,
  },
});

@jclab-joseph
Copy link
Contributor Author

Just keep the browserField, and in the future as well :)

export default defineConfig({
  resolve: {
    // Some libs that can run in both Web and Node.js, such as `axios`, we need to tell Vite to build them in Node.js.
    browserField: false,
  },
});

This code will break in the future if you only use browserField. It is okay?
Wouldn't it be better if it worked even into the future when only mainFields are left?

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 19, 2023

This code will break in the future if you only use browserField. It is okay? Wouldn't it be better if it worked even into the future when only mainFields are left?

browserField interferes with mainFields parsing because browserField has higher priority.

mainFields will always exist(we don't need to set it), and it contains browserField, which is equivalent to ['browser', 'module', 'jsnext:main', 'jsnext'], so browserField will be removed because Vite doesn't just build Web.


It doesn't make sense that you set the mainFields value to the same value as Vite's defalut.

@jclab-joseph
Copy link
Contributor Author

jclab-joseph commented Apr 19, 2023

This code will break in the future if you only use browserField. It is okay? Wouldn't it be better if it worked even into the future when only mainFields are left?

browserField interferes with mainFields parsing because browserField has higher priority.

mainFields will always exist(we don't need to set it), and it contains browserField, which is equivalent to ['browser', 'module', 'jsnext:main', 'jsnext'], so browserField will be removed because Vite doesn't just build Web.

What I'm concerned about is when Vite completely removes browserField functionality.
In "the future", this code will no longer work without mainFields.
browserField is already deprecated and may be removed in the near future.

@caoxiemeihao
Copy link
Member

What I'm concerned about is when Vite completely removes browserField functionality.
In "the future", this code will no longer work without mainFields.

Vite v5.0 will remove browserField, but it has nothing to do with mainFields. There is no effect on forge either :)

@jclab-joseph
Copy link
Contributor Author

What I'm concerned about is when Vite completely removes browserField functionality.
In "the future", this code will no longer work without mainFields.

Vite v5.0 will remove browserField, but it has nothing to do with mainFields. There is no effect on forge either :)

I don't understand yet...

image

It is written that instead of removing browserField in the future browser will be added to mainFields.
Wouldn't that be referring to the browser field again in this case?

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Apr 19, 2023

@jclab-joseph Sorry, I misread! …(⊙_⊙;)…
@BlackHole1 LGTM (◐ˍ◑)

@BlackHole1
Copy link
Member

LGTM. But I don't have merge permission.

Request CR/Merge @erickzhao @VerteDinde

@BlackHole1 BlackHole1 requested review from a team April 25, 2023 01:19
@erickzhao erickzhao changed the title feat(template): fix vite's main process template feat(template-vite): ensure isomorphic modules run in Node Apr 25, 2023
@erickzhao erickzhao changed the title feat(template-vite): ensure isomorphic modules run in Node fix(template-vite): ensure isomorphic modules run in Node Apr 25, 2023
@erickzhao erickzhao changed the title fix(template-vite): ensure isomorphic modules run in Node fix(template-vite): ensure isomorphic modules use the Node.js entrypoint Apr 25, 2023
@erickzhao erickzhao changed the title fix(template-vite): ensure isomorphic modules use the Node.js entrypoint fix(template-vite): ignore browser field for isomorphic packages Apr 25, 2023
@erickzhao erickzhao merged commit 6ac014e into electron:main Apr 25, 2023
@erickzhao
Copy link
Member

Thank you for the contribution! Sorry for the edit history, I tried to make the PR title clearer and more concise.

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

4 participants