Use ESM instead of CJS. Potentially fixes #160 #173
Conversation
| @@ -1,13 +1,12 @@ | |||
| { | |||
| "private": true, | |||
| "type": "module", | |||
There was a problem hiding this comment.
Is node correctly running the bundled files? Just wondering if it correctly detects the package.json, so it can see the type: "module". Otherwise it would see the compiled .js files as CommonJS.
There was a problem hiding this comment.
This is only relevant during the build of the library itself. The compiled files are embedded in the library and are passed to nodejs as a string on the command line. For this reason we have to specify --input-type=module as well when calling node. We cannot depend on any package.json being present at the time of execution, because there could be none and also the compiled file is always ESM regardless of what the package.json in the CWD specifies.
| @@ -97,8 +97,8 @@ | |||
| If we aren't multi-targeting, JavascriptBuild runs before PreBuildEvent. | |||
| --> | |||
| <Target Name="JavascriptBuildWindows" BeforeTargets="DispatchToInnerBuilds" Inputs="@(JavascriptInputs)" Outputs="@(JavascriptOutputs)"> | |||
| <Yarn WorkingDirectory=".\Javascript" Command="run build --env mode=$(Configuration) --env entry=./Servers/OutOfProcess/Http/Http11Server.ts" /> | |||
| <Yarn WorkingDirectory=".\Javascript" Command="run build --env mode=$(Configuration) --env entry=./Servers/OutOfProcess/Http/Http20Server.ts" /> | |||
| <Yarn WorkingDirectory=".\Javascript" Command="run build --ssr ./Servers/OutOfProcess/Http/Http11Server.ts --outDir bin/$(Configuration)" /> | |||
There was a problem hiding this comment.
Could we add a <Yarn WorkingDirectory=".\Javascript" /> task here, to trigger the yarn install.
There was a problem hiding this comment.
do you mean to avoid calling yarn install twice? I'm certainly open to that, although I don't think it makes much of a difference as the second yarn install will return very fast. I was trying in the PR to keep the setup as close to the webpack one as possible to make it easier for @JeremyTCD to approve it. Also I'm not an MSBuild expert, so I'm not completely sure what happens if we add another task here, if that would execute sequentially or in parallel.
There was a problem hiding this comment.
Just seems like the correct way of doing it, but not a biggie if the other way works. :)
|
I have some qualms with using Vite's server-side rendering functionality to generate our bundle because we are using it for an unintended purpose. That said, everything works as expected and our setup is now simpler and more concise than before. Therefore, this pull request looks good overall. Thanks for the contributions! I have released 7.0.0-beta.5 with the changes in this pr. @thebuilder, I have added you to the contributors section of the ReadMe.md file since some of the code in this pull request was originally from your pull request. @aKzenT, I have assigned you write privileges for the repository so that you can merge changes without having to wait for me in the future. |
|
Awesome Jeremy. We can look into using another build tool for bundling the scripts - There's a bunch of them. But as a direct replacement for Webpack, Vite is the closest. |
This moves the JS contained in the project from CJS to ESM, which improves interoperability and potentially fixes #160