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

Using ESM in external preflight function fails #3013

Closed
skorfmann opened this issue Jun 21, 2023 · 5 comments · Fixed by #6157
Closed

Using ESM in external preflight function fails #3013

skorfmann opened this issue Jun 21, 2023 · 5 comments · Fixed by #6157
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@skorfmann
Copy link
Contributor

skorfmann commented Jun 21, 2023

I tried this:

bring cloud;

class Utils {
  init() {}
  extern "./utils.js" toUpperCase(value: str): str;
}

let utils = new Utils();
utils.toUpperCase("world!");

utils.js:

import changecase from 'change-case'

export function toUpperCase(str) {
  return changecase.upperCase(str)
}

This happened:

| [command]/usr/local/bin/wing compile -p /plugins/backend.s3.js -t tf-aws main.w
| Error: ERROR: Cannot use import statement outside a module
|
| target/main.tfaws.166271.tmp/.wing/preflight.js:15
|          }
|           toUpperCase(value)  {
| >>         return (require("/Users/sebastian/projects/monada/wing-github-action/examples/with-dependencies/./utils.js")["toUpperCase"])(value)
|          }
|          static _toInflightType(context) {
|
|     at compile (/usr/local/lib/node_modules/winglang/dist/commands/compile.js:117:19)

I expected this:

It works, as it does for inflight functions (probably due to the esbuild bundling)

Is there a workaround?

use commonjs where possible

Component

Compiler

Wing Version

0.21.5

Wing Console Version

No response

Node.js Version

v18.16.0

Platform(s)

MacOS

Anything else?

Changing the file extension to .mjs or setting "type": "module" doesn't change it either.

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@skorfmann skorfmann added the 🐛 bug Something isn't working label Jun 21, 2023
@Chriscbr Chriscbr added the 🛠️ compiler Compiler label Jun 21, 2023
@skyrpex
Copy link
Contributor

skyrpex commented Jul 12, 2023

I wonder:

  • Do you have a package.json in the root of the project with "type": "module"?
  • Otherwise, did you try renaming your .js file to .mjs?

If none of the above conditions are met, the .js files are treated as commonjs modules.

@skyrpex
Copy link
Contributor

skyrpex commented Jul 12, 2023

I tried renaming to .mjs and got this error:

Failed to compile.

ERROR: require() of ES Module /Users/cristian/Code/test/utils.mjs not supported.
Instead change the require of /Users/cristian/Code/test/utils.mjs to a dynamic import() which is available in all CommonJS modules.

target/index.wsim.116480.tmp/.wing/preflight.js:15
         }
         toUpperCase(value) {
>>         return (require("/Users/cristian/Code/test/./utils.mjs")["toUpperCase"])(value)
         }
         static _toInflightType(context) {

@github-actions
Copy link

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

@MarkMcCulloh
Copy link
Contributor

MarkMcCulloh commented Mar 14, 2024

The core issue is that wing runs preflight as CJS modules. So we require in many places, especially when running user-code via platforms. require in node cannot load ESM modules by default. Part of this is for dumb module-specific reasons, but the other part is that we heavily rely on synchronous code in preflight so import can't be used.

This works fine inflight because we upfront bundle everything to CJS.

The two main solution options:

  1. All in on Migrate @winglang/sdk to ESM #5416, including using import(). This would require async in a lot more places that we don't currently have it.
  2. Use something like https://github.com/unjs/jiti. This provides a mechanism to do synchronous ESM imports in node (unless top-level-await is used).

Both of these things are good ideas regardless, (1) for possible perf and general modernization, (2) to also enable using typescript platforms and preflight externs.

@MarkMcCulloh MarkMcCulloh removed the good first issue Good for newcomers label Mar 14, 2024
mergify bot pushed a commit that referenced this issue Mar 18, 2024
Closes #5868 (except the static part, but that's not critical for the use-case)

Compiling a wing project will now generate .d.ts files for the contract between wing and extern files. These .d.ts files are fully self-contained.

The code for this looks similar to existing dtsification code, but there are couple important distinctions that made it feel useful to separate it:
1. The new code only access to type information rather than the original ASTs
2. The new code is intentionally generating types in the same file as needed instead of producing references

Misc:
- No longer emit any files if any errors occur during compiliation
- Internally change extern to use Utf8PathBuff
- Converted some `examples` externs into typescript (I would love to do the rest, but need #3013)

Note that this may be a breaking change: extern types must be consistent. So in one class you refer to an extern as returning a `str` and in another it returns a Json, that will now be a compiler error.

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@mergify mergify bot closed this as completed in #6157 Apr 12, 2024
mergify bot pushed a commit that referenced this issue Apr 12, 2024
Fixes #3013

~TODO~
- [x] Check benchmarks to make sure happy path perf isn't hurt
- [x] Windows
- [x] ~Add e2e test to verify fix for `"type": "module"`~ Doesn't fix that issue anymore
- [x] Docs to officially sanction using ts in both preflight and inflight via extern

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.70.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants