JorgenVatle / meteor-vite

⚑ Replace Meteor's bundler with Vite for blazing fast build-times
MIT License
32 stars 10 forks source link

node:child_process issues on the client #239

Closed dallman2 closed 1 week ago

dallman2 commented 1 week ago

Hey Jorgen,

I am using meteor-vite in a React codebase, with Meteor 3.0.4. Recently, I have been having trouble actually getting my dev server to launch, with the following error:

Uncaught Error: Cannot find module 'node:child_process'
    at makeMissingError (modules-runtime-hot.js?hash=740f2079be9cde2bf010e27ef9713c36c6491a66:232:12)
    at Module.resolve (modules-runtime-hot.js?hash=740f2079be9cde2bf010e27ef9713c36c6491a66:247:17)
    at Module.moduleLink [as link] (modules.js?hash=c37f9805d87eeac106d00721e598578f852ddc8d:399:25)
    at module (workers.ts:1:43)
    at fileEvaluate (modules-runtime-hot.js?hash=740f2079be9cde2bf010e27ef9713c36c6491a66:386:7)
    at Module.require (modules-runtime-hot.js?hash=740f2079be9cde2bf010e27ef9713c36c6491a66:268:27)
    at mod.require (modules.js?hash=c37f9805d87eeac106d00721e598578f852ddc8d:334:33)
    at Module.moduleLink [as link] (modules.js?hash=c37f9805d87eeac106d00721e598578f852ddc8d:406:22)
    at Helpers.ts:1:57
    at module (Helpers.ts:143:2)

I tracked it down (i think). The import chain is as follows: client.ts -> loading/vite-connection-handler -> api/Endpoints -> utility/Helpers -> workers -> node:child_process

If i understand this correctly, this import chain is causing the client to attempt to import a Node package, which obviously does not work. Is this a bug??

dallman2 commented 1 week ago

line 16 of Endpoints.ts, which is:

const { appId } = getMeteorRuntimeConfig();

seems to be the only place that is using the offending import from Helpers. if this is the issue, can this value be parametrized in another way?

JorgenVatle commented 1 week ago

Huh, that's interesting. Meteor shouldn't have an issue with an import string like that. In fact, there's several other places where it's used. And I don't believe we've changed anything there recently. πŸ˜‚

You're seeing this in your server logs right, not the browser?

JorgenVatle commented 1 week ago

We did however move the package's source files into an src subdirectory with a recent update. Which has impacted only React out of all the other example apps. Because of the new directory structure, the import path for getConfig, referenced in the React example app's react-refresh.js module would cause a similar exception. All with an arguably less than useful error message.

import { getConfig } from 'meteor/jorgenvatle:vite-bundler/loading/vite-connection-handler';

image

Updating the import path

import { getConfig } from 'meteor/jorgenvatle:vite-bundler/src/loading/vite-connection-handler';

And the problem goes away, all of a sudden the package exists again πŸ˜‚

JorgenVatle commented 1 week ago

I am having some trouble replicating the exception you're seeing though. I still have this nagging feeling it is related however. Maybe there's an explicit import for for jorgenvatle:vite-bundler in the application's client code?

I tracked it down (i think). The import chain is as follows: client.ts -> loading/vite-connection-handler -> api/Endpoints -> utility/Helpers -> workers -> node:child_process

If i understand this correctly, this import chain is causing the client to attempt to import a Node package, which obviously does not work. Is this a bug??

The import chain there is very useful. If nothing else works, we could probably just drop the api/Endpoints import as it's mostly only useful for creating optimistic client-side method stubs. Then again, I'm quite dumbfounded on what's going on here.

Meteor shouldn't fail to start because of this import, it should be omitted by the build process unless directly referenced or stubbed out for the client worst case. I haven't seen this show up before in the number of other Meteor projects I work on, hence my amusement. πŸ˜‚

JorgenVatle commented 1 week ago

If you have the time to publish a repository reproducing the issue, I'd be happy to have a closer look at it. πŸ‘

dallman2 commented 1 week ago

Oh! No, to clarify, this error is throwing on the client side. I think because its uncaught, something else is breaking, causing this to be thrown right after:

Uncaught TypeError: Cannot read properties of undefined (reading 'LaunchScreen')
    at global-imports.js?hash=7b2f0a6a117beabe6c36e4feac3d362022a3e574:41:41

I don't think the above error is the true issue, i think it is caused by the original problem.

Funnily enough, I was not getting that issue with the getConfig import. When I updated my path to be 'meteor/jorgenvatle:vite-bundler/src/loading/vite-connection-handler', everything broke and I got the error you had in your server console. I am using version 2.2.0.

EDIT: I pulled in the package locally to make the changes I suggested. They seemed to work, AND I got the error you described in the manner you described. Curious...

EDIT 2: I also had to comment out a few lines (2, 3, 33-37) in Watchers.ts for the same reason as the original issue.

JorgenVatle commented 1 week ago

Oh you're right, the change to the src directory might still pending release. πŸ˜…

I wonder if it could be an issue with the environment possibly? If you cloned this repository and ran the examples/react app, does the same error appear? I think I've seen an instance where Meteor would launch with a Node.js version not matching the one it ships with.

Out of pure curiosity I loaded up a Meteor app and placed an unused node:child_process import in the client's main module, without a single complaint from Meteor or in the browser. Just got compiled to undefined. Though, for context, this was done within my main workspace for this repository, so there might just be some pollution from other local dependencies. I'll double-check in a fresh environment and get back to you.

JorgenVatle commented 1 week ago

Created a fresh workspace on Ubuntu using this repository as a base, added the import directly to the client mainModule, and still no complaints to be seen. πŸ€”

This is using the latest public releases, meteor-vite@1.12.0 and jorgenvatle:vite-bundler@2.2.0. Ironically enough, I did have to revert the commit from earlier today fixing the getConfig import path to get it to run with those releases. πŸ˜‚

The release that's lined up now with the change to the directory where getConfig() lives was marked as a patch release. Because of the fact that would almost certainly break all React apps based on that example on update, paired with the inaccurate error message, I'm really glad you brought up this issue. Even though it might not be directly related.


image image

dallman2 commented 1 week ago

I wonder if the bundler was compiling out the import entirely? Did you try invoking the method after importing it into the client module? At the very least, that likely should not work, as node packages are not available on the client side. Edit: That is very strange. You should not be able to import node internals on the client, and they definitely will not be defined if you do. My guess is somehow, your code was getting bundled, but the node:xyz packages were not being bundled.

In any case, we determined that the issue was only happening on version 2.2.0. it started after we upgraded from 2.0.1. the version of the npm package does not seem to matter.

Finally, I am having trouble building using the latest version as well. For whatever reason, no chunk called meteor-entry is being generated by the vite build (I have server builds enabled). I'll open that as a separate issue when I return from the coffee shop:)

240 is the issue for this

JorgenVatle commented 1 week ago

I wonder if the bundler was compiling out the import entirely? Did you try invoking the method after importing it into the client module? At the very least, that likely should not work, as node packages are not available on the client side.

Yeah, that is the idea. There really shouldn't be an instance where the error you got should be possible to have on the client. Yet, it did show up on the client. Again, I am amused at how strange working with Meteor can be at times.

Though considering it's all client-side, you could check and make sure node-meteor-stubs is installed and up to date. Not sure how much work the npm package does by itself, but could be worth to check. It comes by default with all Meteor projects unless I'm mistaken.

dallman2 commented 1 week ago

the node stubs thing seems to fix the issue, but i still cannot build :(

JorgenVatle commented 1 week ago

Aha, glad to hear it. Thanks for clarifying. I'll be sure to add some checks for that for future releases. πŸ™Œ

the node stubs thing seems to fix the issue, but i still cannot build :(

Turn off experimental features in your Vite config and I suspect it will build just fine. 🀞

dallman2 commented 1 week ago

i had turned it off but still no luck