fix: remove async imports

[marcpalm]

Hey, I just made a Pull Request!

Proposed solution for #24864

How to test:

1. add to packages/app/package.json in devDependencies

    "@vitejs/plugin-react": "^4.0.4",
    "cross-env": "^7.0.0",
    "vite": "^4.4.9",
    "vite-plugin-html": "^3.2.0",
    "vite-plugin-node-polyfills": "^0.21.0"

2. Run yarn from the root
3. Run EXPERIMENTAL_VITE=true yarn workspace example-app start

The error is only reproducible in https://github.com/backstage/demo

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• Added or updated documentation
• Tests for new functionality and regression tests for bug fixes
• Screenshots attached (for UI changes)
• All your commits have a Signed-off-by line in the message. (more info)

[backstage-goalie]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/cli	packages/cli	patch	v0.26.6-next.0

[benjdlambert]

Is this a duplicate of #24863?

[benjdlambert]

I'm not sure we want to do this yet. Would mean that we would have to ship the vite dependencies with the CLI even if you're not using it which is not a great experience when it duplicates a lot of versions. I'm also not sure how this would actually even fix the issue.

It could be the actual fix for this error message might be something like this: remix-run/remix#6562

[backstage-goalie]

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

[marcpalm]

I'm not sure we want to do this yet. Would mean that we would have to ship the vite dependencies with the CLI even if you're not using it which is not a great experience when it duplicates a lot of versions.

No, this is a conditional import, not an unconditional import like at the top level of a file.

I'm also not sure how this would actually even fix the issue.

The problem is that import(...) does not correctly resolve (my guess). This fixes this.

It could be the actual fix for this error message might be something like this: remix-run/remix#6562

I would be surprised. Vite was working before for us, we upgraded to the new backend system, and it stopped working for us.

[marcpalm]

I added a description how to test, but the error is not reproducible here, only in https://github.com/backstage/demo.

Can somebody please trigger a prerelease, then I can test this with the demo-site? I have only tested this with modifying the dist folder in the node_modules yet ;-)

[benjdlambert]

Backing up a bit here, I guess I wanna work out why this doesn't work in the demo site when you add the vite dependencies, but the same code works in this repo. My guess is that it's something to do with some dependencies in packages/app that are being loaded in the demo site but not in the main repo, or it's some mismatch in the vite packages?

@asyncapi/parser seems to be a dependency on @backstage/plugin-api-docs, but that seems to be used in both places, so it's all very strange.

[0] Loaded config from app-config.yaml
[0]
[0]  WARN  files in the public directory are served at the root path.
[0] Instead of /public/index.html, use /index.html.
[0]
[0] ✘ [ERROR] No matching export in "../../node_modules/node-stdlib-browser/esm/mock/empty.js" for import "readFile"
[0]
[0]     ../../node_modules/@asyncapi/parser/esm/from.js:10:9:
[0]       10 │ import { readFile } from 'fs';
[0]          ╵          ~~~~~~~~

I put together a fix for the demo site without using require, which happens to be pretty similar to the code we're using internally as we had some problems with the polyfills before. You might be able to use the same yarn patch method to test your changes there too, but to be honest I don't understand how that even fixes it.

EDIT: I've just had a thought looking at the stack trace that it's loading the esm exports for the stdlib-browser polyfill. I wonder if that's different between the demo site and this repo.

[marcpalm]

My guess is also only vague at the moment: the import(...) needs to be interpreted by typescript and transpilled. The resolution is sensitive to module resolution, which can be influenced by certain files, e.g. module-field in package.json or via the tsconfig.json.

By directly using require(...), I do not need to understand all this however, because require is not transpiled.

[marcpalm]

You might be able to use the same yarn patch method to test your changes there too, but to be honest I don't understand how that even fixes it.

EDIT: I've just had a thought looking at the stack trace that it's loading the esm exports for the stdlib-browser polyfill. I wonder if that's different between the demo site and this repo.

This does work. Just results in a warning (also for my patch by the way) -

╭─... ~/repos/backstage/demo ‹blam/test-vite-fix●› 
╰─$ EXPERIMENTAL_VITE=true yarn workspace app start
Loaded config from app-config.yaml

 WARN  files in the public directory are served at the root path.                               13:16:10
Instead of /public/index.html, use /index.html.

 WARN  1:16:10 PM [vite] Failed to load source map for /@fs/Users/palm/repos/backstage/demo/node_modules/graphiql/graphiql.css.

[github-actions]

Uffizzi Cluster pr-24865 was deleted.

[marcpalm]

Edit: Both patches - yours and mine - seem to work, the above warning can be ignored.

[benjdlambert]

OK, I think that this probably makes sense.

I think that we're going to have to change this though for the release of Vite 5+ as they're deprecating their cjs API, which means no more require from what I gather, so we will have to go back to await import at least for the vite dependencies at a later date, maybe the polyfill one we can keep as require, or take inspiration from my demo site patch I guess.

let's go with this for now, I realised that my concerns of this throwing errors shouldn't be a thing as it's conditionally called. 🤞

[github-actions]

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.28.0 release, scheduled for Tue, 18 Jun 2024.

[marcpalm]

OK, I think that this probably makes sense.

I think that we're going to have to change this though for the release of Vite 5+ as they're deprecating their cjs API, which means no more require from what I gather, so we will have to go back to await import at least for the vite dependencies at a later date, maybe the polyfill one we can keep as require, or take inspiration from my demo site patch I guess.

let's go with this for now, I realised that my concerns of this throwing errors shouldn't be a thing as it's conditionally called. 🤞

Thanks for merging. Your comment applies to code run inside the vite-server only, right? We are configuring and starting it here.

[marcpalm]

Ah okay, educated myself. You are right. It is deprecated in v4 and will removed in v5.