feat: handle node builtins better #111
Conversation
This change is designed to handle the node builtins more effectively, since they were moved to `node:xxx`; Fixes garronej#110
There was a problem hiding this comment.
Hey @dancrumb,
First, I want to express my deep appreciation for your valuable time and effort.
Having proper support for environment variables is crucial as it will enable a large number of modules to be denoified.
On a PR with such an extensive DIFF, it's inevitable that I will have some comments. So, here's my review.
To summarize, it seems that the essential task to support environment variables involves detecting the use of process and appending import process from "process"; at the top of the file.
We then let denoifyImportExportStatement.ts prepend node:.
| @@ -2,26 +2,21 @@ | |||
|
|
|||
| import { join as pathJoin, normalize as pathNormalize, dirname as pathDirname, basename as pathBasename } from "path"; | |||
| import { getIsDryRun } from "./lib/getIsDryRun"; | |||
| import * as fs from "fs"; | |||
| import * as fs from "fs/promises"; | |||
There was a problem hiding this comment.
I'd rather use sync version of for scripting since working synchronously is easyer te reason about and ther's no performance gain in going with async.
| "getConfigFileRawContent": fileBasename => { | ||
| const filePath = pathJoin(moduleDirPath, fileBasename); | ||
| if (!fs.existsSync(filePath)) { | ||
| return Promise.resolve(undefined); | ||
| } | ||
| return Promise.resolve(fs.readFileSync(filePath).toString("utf8")); | ||
| } | ||
| "getConfigFileRawContent": fileBasename => getLocalFileContents(pathJoin(moduleDirPath, fileBasename)) |
There was a problem hiding this comment.
Here I prerer having seeing explicitely what I'm doing.
I don't think it's woth it to abstart this away.
That said I admit that it would have been more clean to write:
"getConfigFileRawContent": async fileBasename => {
const filePath = pathJoin(moduleDirPath, fileBasename);
if (!fs.existsSync(filePath)) {
return undefined;
}
return fs.readFileSync(filePath).toString("utf8");
}By having explicitely a test case for if the file dosen't exist we explicitely aknoelage that it's ok if the file dosen't exist.
| @@ -32,10 +27,13 @@ const { getDenoifyOutDir } = (() => { | |||
| return pathNormalize(toPosix(denoifyOut)); | |||
| } | |||
|
|
|||
| function getTsconfigOutDir(params: { moduleDirPath: string }): string | undefined { | |||
| async function getTsconfigOutDir(params: { moduleDirPath: string }): Promise<string | undefined> { | |||
There was a problem hiding this comment.
There's no performance gain in making this function async.
I'm not saying we shouldn't favor async when writing backend code but for procedural script it's not worth it IMO.
| if (tsconfigJson === undefined) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
I think here we are in the case we expect the tsconfig to be present.
To write a transcription of the code that was overwrite you would wirte something like:
const tsconfigJson = await getLocalFileContents(pathJoin(moduleDirPath, "tsconfig.json"));
assert(tsconfigJson !== undefined, "There's no tconfig.json"); This way after the assert statement typescript would know that tsconfigJson isn't undefined (because otherwise we would have had a runtime exception).
But again, I don't think abtracting away the fs API is desirable. Most node developper are familiar with the fs API spect and it's well documented. It makes it easyer to reson about than if we introduce our wrapper.
| @@ -49,23 +47,15 @@ const { getDenoifyOutDir } = (() => { | |||
| async function getDenoifyOutDir(params: { moduleDirPath: string }) { | |||
There was a problem hiding this comment.
I think I prefer the previous factor of this function, I see how it might come off as noisy but there is acutally a reasonling about behind this scroping.
We want to avoir leaking variable declaration as much as posible in order to be able to reason about independent unit of code.
In your version we don't know if denoifyOutDir will be used later on, it's still visible.
An other thing is, in the contrôle flow we want to 1) deal with the exception 2) get the actual meat of the function.
It's closer from what the actual reasoning is:
"If the denoify out dir has not been explicitely specified then we assert it from the ts-config out dir, if there is no tsconfigOutDir then we don't know."
src/lib/resolveNodeModuleToDenoModule/finderFactories/denoPortImportFinderFactory.ts
Outdated
Show resolved
Hide resolved
| const denoPorts = getDenoPorts(params.userProvidedPorts); | ||
| const { log } = params; | ||
|
|
||
| return async (nodeModuleName: string, version: string): Promise<Result | null> => { |
There was a problem hiding this comment.
If we want to introduce a factory we would always respect this formating:
function myFunctionFactory(params: Params){
const { foo, bar } = params;
function myFunction(params: Params){
}
return { myFunction };
}And it shoud be used like this:
const { myFunction } = myFunctionFactory(...);
| export const dependencyImportUrlFinderFactory = (params: { | ||
| userProvidedPorts: Dependencies; | ||
| dependencies: Dependencies; | ||
| devDependencies: Dependencies; | ||
| }) => { |
There was a problem hiding this comment.
Same here, we prefer the OF function syntax. https://deno.com/manual@v1.34.3/references/contributing/style_guide#top-level-functions-should-not-use-arrow-syntax
| { getInstalledVersionPackageJson }: ReturnType<typeof getInstalledVersionPackageJsonFactory>, | ||
| { userProvidedPorts = {}, dependencies = {}, devDependencies = {}, log = console.log }: FactoryParams = {} |
There was a problem hiding this comment.
Yeah, not sure it make sense, the way I wrote the prototype sucks, I'll give you that but I'm not sold of have in having two parameter.
Also I I stated before I would like to avoid deconstricting in the function prototype and avoid allowing not to provide the parameter. Defaults are for usage convignence. We only use most of our function once, we want type safety above all esle.
| if (nodeToDenoModuleResolutionResult.result === "KNOWN BUILTIN") { | ||
| return stringify(ParsedImportExportStatement.ParsedArgument.stringify(parsedImportExportStatement.parsedArgument)); | ||
| } | ||
|
|
||
| if (nodeToDenoModuleResolutionResult.result === "UNKNOWN BUILTIN") { | ||
| return stringify(`node:${ParsedImportExportStatement.ParsedArgument.stringify(parsedImportExportStatement.parsedArgument)}`); |
There was a problem hiding this comment.
As you can see.
Denoify was already resolving built in like:
https://github.com/garronej/my_dummy_npm_and_deno_module/blob/master/deno_dist/tools/getPackageJsonName.ts#L30-L32
I impelemnted it in a very hacky way, when I realized that there was not any STD module I just added a case for when:
- The module isn't in
node_modules. - It's used in the code (there is an
import "xxxx";) - We don't have a port for it. (This condition is always met since all my links are dead)
Then we just preprend node: and we call it a day.
Now that you have properly handled the case I think maybe the "KNOWN BUILTIN" vs "UNKNOWN BUILTIN" dosen't make sence anymore.
Why don't we introduce a "NOT IN NODE_MODULE" get rid of the knownPorts.ts->builtins and just slap node: prefix?
What do you think?
| project: true, | ||
| tsconfigRootDir: __dirname | ||
| }, | ||
| rules: { |
There was a problem hiding this comment.
I added this to capture some of the conventions that you raised - I think conventions are a vital part of collaborative work, so hopefully this will help to keep people on the same page
| rules: { | ||
| "no-restricted-imports": ["error", "fs/promises"], | ||
| "func-style": ["error", "declaration", { "allowArrowFunctions": true }], | ||
| "max-params": ["error", 1] |
There was a problem hiding this comment.
This isn't ideal, but it should help remind folks to use parameter objects.
There was a problem hiding this comment.
Once consequence was that I needed to refactor a couple of functions in the codebase to align with this, but nothing major
|
Hello @dancrumb, Firstly, I'd like to emphasize that I genuinely acknowledge and appreciate your breadth of experience as a programmer and your significant contributions to this project. However, recently I encountered a substantial issue when merging a PR with a large DIFF. It's only natural that errors may occasionally occur even amongst the most competent software engineers. An example of this can be found here. The larger the diff, the more challenging it becomes to identify any potential errors. Therefore, if you are amenable, I would recommend you to submit smaller, more atomic PRs. This would certainly facilitate the review process. Regarding the specific issue we're addressing at present, I believe this approach would be most appropriate. Consequently, I will close this PR and invite you to submit another one, related to: #110 Thank you for your understanding. |
This change is designed to handle the node builtins more effectively since they were moved to
node:xxx;Sorry for all the churn around this. :/
It turns out that Deno provides access to Node builtins via
node:xxximports.This PR updates how we handle and detect imports of node builtins.
I also did some refactoring to help me understand what denoify is doing with imports. I broke things up and hopefully, that makes things a little clearer. I'm happy to rollback the refactors if desired.
Fixes #110