From 07c6bd84d4c9c029d1ae286d4f3c76cb37fdf41f Mon Sep 17 00:00:00 2001 From: Liad Yosef Date: Fri, 1 Nov 2024 12:53:39 +0200 Subject: [PATCH] feat(import): detect node built-ins (#6609) This PR adds the ability to detect Node built-ins that are being imported. **Notes:** - We currently detect all imports in project files, without checking if the importing file is in the import chain of the app itself. This _may_ result in false positives, if one of the files in the project that does not belong to the app imports a Node module. - For that reason - currently this only triggers a "warning" and not a critical error. - We compare the built-ins that we find with the project dependencies, in case some of them are shimmed. - We're using a static list taken from here: https://github.com/sindresorhus/builtin-modules. We can't use the package directly since it is an ESM module, and our tests cannot import it, and in any case all it does is exporting this JSON. If you clone this branch you can test it using http://localhost:8000/p?accessLevel=public&clone=liady/utopia-vite-project - I added an `import * as fs from 'fs'` there to test this functionality. (don't forget to turn on the `Import Wizard` feature flag) image **Manual Tests:** I hereby swear that: - [X] I opened a hydrogen project and it loaded - [X] I could navigate to various routes in Play mode --- .../requirements/builtin-modules.json | 55 +++++++++++++++++++ .../requirement-server-packages.ts | 40 +++++++++++++- .../requirements/requirements.spec.ts | 55 +++++++++++++++++++ 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 editor/src/core/shared/import/project-health-check/requirements/builtin-modules.json diff --git a/editor/src/core/shared/import/project-health-check/requirements/builtin-modules.json b/editor/src/core/shared/import/project-health-check/requirements/builtin-modules.json new file mode 100644 index 000000000000..e3bfb90c64a4 --- /dev/null +++ b/editor/src/core/shared/import/project-health-check/requirements/builtin-modules.json @@ -0,0 +1,55 @@ +[ + "assert", + "assert/strict", + "async_hooks", + "buffer", + "child_process", + "cluster", + "console", + "constants", + "crypto", + "dgram", + "diagnostics_channel", + "dns", + "dns/promises", + "domain", + "events", + "fs", + "fs/promises", + "http", + "http2", + "https", + "inspector", + "inspector/promises", + "module", + "net", + "os", + "path", + "path/posix", + "path/win32", + "perf_hooks", + "process", + "punycode", + "querystring", + "readline", + "readline/promises", + "repl", + "stream", + "stream/consumers", + "stream/promises", + "stream/web", + "string_decoder", + "timers", + "timers/promises", + "tls", + "trace_events", + "tty", + "url", + "util", + "util/types", + "v8", + "vm", + "wasi", + "worker_threads", + "zlib" +] diff --git a/editor/src/core/shared/import/project-health-check/requirements/requirement-server-packages.ts b/editor/src/core/shared/import/project-health-check/requirements/requirement-server-packages.ts index 7b18d7cf4f03..fa86fcf80da8 100644 --- a/editor/src/core/shared/import/project-health-check/requirements/requirement-server-packages.ts +++ b/editor/src/core/shared/import/project-health-check/requirements/requirement-server-packages.ts @@ -4,13 +4,17 @@ import { type RequirementCheck, type RequirementCheckResult, } from '../utopia-requirements-types' -import { getProjectDependencies } from '../../../../../components/assets' +import { getProjectDependencies, walkContentsTree } from '../../../../../components/assets' +import { isParseSuccess, isTextFile } from '../../../../../core/shared/project-file-types' +import builtinModules from './builtin-modules.json' const serverPackagesRestrictionList: RegExp[] = [/^next/, /^remix/, /^astro/, /^svelte/] export default class CheckServerPackages implements RequirementCheck { check(projectContents: ProjectContentTreeRoot): RequirementCheckResult { const projectDependencies = getProjectDependencies(projectContents) ?? {} + + // check for server packages in dependencies const serverPackages = Object.keys(projectDependencies).filter((packageName) => serverPackagesRestrictionList.some((restriction) => restriction.test(packageName)), ) @@ -21,9 +25,43 @@ export default class CheckServerPackages implements RequirementCheck { resultValue: serverPackages.join(', '), } } + + // check for node builtins in imports + const nodeBuiltins: string[] = [] + walkContentsTree(projectContents, (fullPath, file) => { + if (isTextFile(file)) { + const parseResult = file.fileContents.parsed + if (isParseSuccess(parseResult)) { + for (const importSource of Object.keys(parseResult.imports)) { + // if it's a node builtin and not shimmed as a dependency, add it to the list + if (isBuiltinModule(importSource) && projectDependencies[importSource] == null) { + nodeBuiltins.push(importSource) + } + } + } + } + }) + if (nodeBuiltins.length > 0) { + return { + resolution: RequirementResolutionResult.Partial, + resultText: 'Node built-ins found', + resultValue: nodeBuiltins.join(', '), + } + } return { resolution: RequirementResolutionResult.Passed, resultText: 'No server packages found', } } } + +const moduleSet = new Set(builtinModules) +const NODE_PROTOCOL = 'node:' +function isBuiltinModule(moduleName: string) { + let moduleNameWithoutNodeProtocol = moduleName + if (moduleName.startsWith(NODE_PROTOCOL)) { + moduleNameWithoutNodeProtocol = moduleName.slice(NODE_PROTOCOL.length) + } + + return moduleSet.has(moduleNameWithoutNodeProtocol) +} diff --git a/editor/src/core/shared/import/project-health-check/requirements/requirements.spec.ts b/editor/src/core/shared/import/project-health-check/requirements/requirements.spec.ts index 7ad7c0e6dfa1..05635d128861 100644 --- a/editor/src/core/shared/import/project-health-check/requirements/requirements.spec.ts +++ b/editor/src/core/shared/import/project-health-check/requirements/requirements.spec.ts @@ -11,6 +11,7 @@ import { DefaultPackageJson } from '../../../../../components/editor/store/edito import { getPackageJson } from '../../../../../components/assets' import CheckStoryboard from './requirement-storyboard' import CheckServerPackages from './requirement-server-packages' +import { parseProjectContents } from '../../../../../sample-projects/sample-project-utils.test-utils' describe('requirements checks', () => { describe('project language', () => { @@ -219,5 +220,59 @@ describe('requirements checks', () => { const result = check.check(projectContents) expect(result.resolution).toBe(RequirementResolutionResult.Critical) }) + + it('should return partial for a project with node builtins', () => { + const check = new CheckServerPackages() + const project = simpleDefaultProject({ + additionalFiles: { + '/src/app.js': textFile( + textFileContents( + `import { readFileSync } from 'fs' + import * as crypto from 'node:crypto'`, + unparsed, + RevisionsState.CodeAhead, + ), + null, + null, + 0, + ), + }, + }) + const parsedProjectContents = parseProjectContents(project.projectContents) + const result = check.check(parsedProjectContents) + expect(result.resolution).toBe(RequirementResolutionResult.Partial) + expect(result.resultValue).toBe('fs, node:crypto') + }) + + it('should return success for a project with node builtins that are shimmed', () => { + const check = new CheckServerPackages() + const project = simpleDefaultProject({ + additionalFiles: { + '/src/app.js': textFile( + textFileContents( + `import { readFileSync } from 'fs'`, + unparsed, + RevisionsState.CodeAhead, + ), + null, + null, + 0, + ), + '/package.json': textFile( + textFileContents( + JSON.stringify({ dependencies: { fs: '1.0.0' } }, null, 2), + unparsed, + RevisionsState.CodeAhead, + ), + null, + null, + 0, + ), + }, + }) + const parsedProjectContents = parseProjectContents(project.projectContents) + const result = check.check(parsedProjectContents) + expect(result.resolution).toBe(RequirementResolutionResult.Passed) + }) }) })