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

Switch to use nodenext module resolution #236640

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Switch to use nodenext module resolution #236640

wants to merge 4 commits into from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Dec 19, 2024

This better enforces that imports end with a file extension and also use relative paths

@mjbvz
Switch to use nodenext module resolution
fca5600 
This better enforces that imports end with a file extension and also use relative paths
@mjbvz mjbvz added this to the January 2025 milestone Dec 19, 2024
@mjbvz mjbvz enabled auto-merge December 19, 2024 20:04
Yoyokrazy
Yoyokrazy previously approved these changes Dec 19, 2024
View reviewed changes
@mjbvz mjbvz mentioned this pull request Dec 19, 2024
Open
bpasero
bpasero reviewed Dec 20, 2024
View reviewed changes
src/bootstrap-node.ts Outdated Show resolved Hide resolved
bpasero
bpasero reviewed Dec 21, 2024
View reviewed changes
src/vs/workbench/contrib/webview/browser/webviewMessages.d.ts
@@ -3,8 +3,8 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { IMouseWheelEvent } from 'vs/base/browser/mouseEvent';
import type { WebviewStyles } from 'vs/workbench/contrib/webview/browser/webview';
import type { IMouseWheelEvent } from '../../../../base/browser/mouseEvent.ts';
Copy link
Member

Choose a reason for hiding this comment

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

@mjbvz I think here it should also be .js

@BABA983
Copy link
Contributor

BABA983 commented Dec 21, 2024

It seems required to set option module to NodeNext. But if we set "module": "NodeNext" the tsc will generate export {} at the end of the file in preload.js which results in error syntax since electron does not support esm in sandboxed preload script.
CleanShot 2024-12-22 at 00 28 08@2x

@bpasero
Copy link
Member

bpasero commented Dec 21, 2024 •
edited
Loading

It seems required to set option module to NodeNext. But if we set "module": "NodeNext" the tsc will generate export {} at the end of the file in preload.js which results in error syntax since electron does not support esm in sandboxed preload script.

Yes, thats a bug that I reported but was closed as "works as intended": microsoft/TypeScript#60074

@BABA983
Copy link
Contributor

BABA983 commented Dec 22, 2024

It seems required to set option module to NodeNext. But if we set "module": "NodeNext" the tsc will generate export {} at the end of the file in preload.js which results in error syntax since electron does not support esm in sandboxed preload script.

Yes, thats a bug that I reported but was closed as "works as intended": microsoft/TypeScript#60074

Even if you don't use an import statement when the module is set to NodeNext in tsconfig.json it still generates export {} at the end of the file.

CleanShot 2024-12-22 at 13 44 06@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpasero bpasero bpasero left review comments

@mScott224 mScott224 mScott224 approved these changes

@Yoyokrazy Yoyokrazy Yoyokrazy left review comments

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

Assignees

@mjbvz mjbvz

Labels
None yet
Projects
None yet
Milestone
January 2025
Development

Successfully merging this pull request may close these issues.
5 participants
@mjbvz @BABA983 @bpasero @Yoyokrazy @mScott224