Conversation
dobbs
left a comment
dobbs
left a comment
There was a problem hiding this comment.
I was hoping that we'd be able to switch to imports instead of requires, so thanks for this effort. I've looked over the changes somewhat closely but haven't tried running the code or tests locally yet.
| import path from 'node:path' | ||
| import { randomBytes } from 'node:crypto' | ||
| import { fileURLToPath } from 'node:url' | ||
| import { dirname } from 'node:path' |
There was a problem hiding this comment.
Can line 12 and line 15 be combined, since both are coming from node:path?
Maybe this would work?
import {default:path, dirname} from 'node:path';
There was a problem hiding this comment.
For sure. I still consider this an exploratory branch rather than something I'd offer to merge right now. Many things like this to tidy up. Thank you for looking it over and commenting.
| argv.id ||= path.join(argv.status, 'owner.json') | ||
| argv.uploadLimit ||= '5mb' | ||
| argv.cookieSecret ||= require('crypto').randomBytes(64).toString('hex') | ||
| argv.cookieSecret ||= randomBytes(64).toString('hex') |
There was a problem hiding this comment.
Thanks for moving the inline require() to an import() above.
| // | ||
| // const async = require('async') | ||
| // | ||
| // // const random_id = require('./random_id.cjs') |
There was a problem hiding this comment.
There are several unused imports throughout the files. As far as I can tell, random_id is not used in pages.
| packageDir: path.join(__dirname, '..', 'node_modules'), | ||
| security_legacy: true, | ||
| }) | ||
| const page = require('../lib/page')(argv) |
There was a problem hiding this comment.
I like this spot too where you pull the inline require() up among the other imports.
| const __dirname = path.dirname(__filename) | ||
|
|
||
| // CommonJS server module (.cjs) | ||
| const server = await import('../index.js') |
There was a problem hiding this comment.
Just noticing out loud how much this dynamic import() stands out against the background of the other static imports. I think this lends itself to clarify some utility in switching from require() to import. This difference is visible here in a way that it wasn't before.
There was a problem hiding this comment.
Thanks again for comments. Some things to note with this repo:
- 'node test' and 'deno task test' both pass on my machine but I won't be surprised if there are version dependencies.
- I used ChatGPT to do the dynamic import conversions in server, so it's only as good as the test cases that test it. For example, the version endpoint is currently commented out in the ESM version of server.js in this tree. I tried adding a test to the test suite for testing the version cases, but I'm not sure that the current tests quite allow for that yet.
There was a problem hiding this comment.
Note that server then has two await imports too. One for the database and one for the security module. Changing things around to only need static imports seem like it would be a desirable simplification.
|
Rather than risk this PR going AWOL, like a previous one. After merging decaffeinate into main I've merged this into decaffeinate and renamed it ESM. |
3 participants
I'm not sure if the the branch is ready for ESM or if that's even desirable, but did a bit today and figured I'd at least share the progress I made.
npm testpasses (in my environment) and I ran the format command somewhat recently...