Skip to content

[wasm] Remove support for CommonJS#72865

Merged
pavelsavara merged 2 commits intodotnet:mainfrom
pavelsavara:wasm_drop_cjs
Jul 28, 2022
Merged

[wasm] Remove support for CommonJS#72865
pavelsavara merged 2 commits intodotnet:mainfrom
pavelsavara:wasm_drop_cjs

Conversation

@pavelsavara
Copy link
Member

Fixes #63337

It will need changes in the SDK unit tests

@pavelsavara pavelsavara added this to the 7.0.0 milestone Jul 26, 2022
@pavelsavara pavelsavara self-assigned this Jul 26, 2022
@ghost
Copy link

ghost commented Jul 26, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #63337

It will need changes in the SDK unit tests

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

pavelsavara commented Jul 27, 2022

I can use v8 --module to get rid of src\mono\sample\wasm\console-v8-es6\v8shim.cjs and make test-main.js ES6
In src\mono\wasm\host\JSEngineHost.cs too. I next PR.

@pavelsavara pavelsavara marked this pull request as ready for review July 27, 2022 15:34
@pavelsavara pavelsavara requested a review from radekdoulik July 27, 2022 17:26
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

<_EmccCommonFlags Include="$(_DefaultEmccFlags)" />
<_EmccCommonFlags Include="$(EmccFlags)" />
<_EmccCommonFlags Include="-s EXPORT_ES6=1" Condition="'$(WasmEnableES6)' != 'false'" />
<_EmccCommonFlags Include="-s EXPORT_ES6=1" />
Copy link
Member

Choose a reason for hiding this comment

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

I think this flag needs to be moved to be just for compiling? I saw emcc warnings that it is not relevant or used when linking. Doesn't block this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created follow-up issue for this #72990

<CMakeConfigurationLinkFlags Condition="'$(Configuration)' == 'Debug'" >$(CMakeConfigurationEmccFlags)</CMakeConfigurationLinkFlags>
<CMakeConfigurationLinkFlags Condition="'$(Configuration)' == 'Release'">-O2</CMakeConfigurationLinkFlags>
<CMakeConfigurationLinkFlags Condition="'$(WasmEnableES6)' != 'false'">$(CMakeConfigurationLinkFlags) -s EXPORT_ES6=1</CMakeConfigurationLinkFlags>
<CMakeConfigurationLinkFlags>$(CMakeConfigurationLinkFlags) -s EXPORT_ES6=1</CMakeConfigurationLinkFlags>
Copy link
Member

Choose a reason for hiding this comment

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

super nit:

Suggested change
<CMakeConfigurationLinkFlags>$(CMakeConfigurationLinkFlags) -s EXPORT_ES6=1</CMakeConfigurationLinkFlags>
<CMakeConfigurationLinkFlags >$(CMakeConfigurationLinkFlags) -s EXPORT_ES6=1</CMakeConfigurationLinkFlags>

@pavelsavara pavelsavara merged commit 8fdec7f into dotnet:main Jul 28, 2022
@pavelsavara pavelsavara deleted the wasm_drop_cjs branch July 29, 2022 09:16
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[wasm] Drop support for CommonJS and WasmEnableES6

2 participants