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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b85e25
Adjusts bundler output to work with Node.
gunsch May 21, 2024
4bdc20f
more comments
gunsch May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/bundler/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

// This is a light hack that satisfies two different runtime needs:
// * runtime-lite needs `exports` to be defined and aliased to `module.exports` like so.
// * Node wraps the file in a function that provides `module` and `exports` already, so
// needs them not to be overwritten.
return src.replace(
/^"use strict";/,
'"use strict"; module.exports = {}; const {exports} = module;'
'"use strict"; if (!module.exports) { module.exports = {}; var exports = module.exports; }\n'
)
}

Expand Down