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

Adjusts bundler output to work with Node. #20

Merged
merged 2 commits into from
May 21, 2024
Merged

Adjusts bundler output to work with Node. #20

merged 2 commits into from
May 21, 2024

Conversation

gunsch
Copy link
Collaborator

@gunsch gunsch commented May 21, 2024

💸 TL;DR

Adjusts bundler output to also work with Node.

📜 Details

  • In Node, require calls are wrapped in a function that provides modules and exports. Redefining them caused the app to not load.
  • Deleting the src.replace line modified here fixed Node, but broke runtime-lite, which needs those definitions shimmed in.
  • The resulting code is a compromise that works in both cases, at the cost of an ugly var in the generated code. Sorry about that.

🧪 Testing Steps / Validation

  • Testing a simple Devvit with Play that registers a custom post until I got it to fully load and render the post!

@gunsch gunsch requested review from niedzielski, logan and RGood May 21, 2024 02:36
@@ -21,10 +21,14 @@ export function compile(env: tsvfs.VirtualTypeScriptEnvironment): string {
const src =
env.languageService.getEmitOutput(appEntrypointFilename).outputFiles[0]
?.text ?? ''
// Adapt bundle CommonJS output to format expected by runtime-lite.
// Adapt bundle CommonJS output to format expected by our runtimes.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for this beefy comment and PR message. I didn't understand why var specifically was needed--it's hoisted outside the block but why? some issue with @devvit/public-api? I know the code is cryptic by nature but I see it and immediately think it's safe to rewrite `"use strict"; module.exports ??= {}; const {exports} = module; }\n'. anything you can do to make this clearer would be appreciated.

the other thing I was thinking is that changing app code is hard but changing runtime-lite or the compute @devvit/public-api bundle is possible if wanted.

Copy link

Choose a reason for hiding this comment

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

When we load a cjs file into a module in node, the contents are basically wrapped like this:

(function(exports, require, module, __filename, __dirname) {
// Module code actually lives in here
}); 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

beefiness++

added more comment, hopefully that explains better

we could change runtime-lite and drop this src.replace call entirely, though that's a little outside the scope I wanted to dig into. we can't fix this on the compute side though without some ugly Node hackery (i.e. monkey-patching the contents of require('module').wrapper)

@gunsch gunsch merged commit 788f519 into main May 21, 2024
1 check passed
@gunsch gunsch deleted the bundler-fix branch May 21, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants