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

Support for non-relative import paths (explicit file extensions). #377

Open
virtuallyunknown opened this issue Feb 20, 2023 · 17 comments
Open

Comments

@virtuallyunknown
Copy link

Greetings!

So I was wondering if support for non-relative import paths in the generated files could be added. To illustrate the problem and exactly what I mean, consider the following tsconfig.json:

{
    "compilerOptions": {
        "module": "NodeNext",
        "moduleResolution": "node16",
        "target": "ESNext",
    },
}

When moduleResolution is set to node16 or nodenext, you need to explicitly specify file extensions in your import paths.

image

❌ Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './Actor.js'?

I found a workaround for this problem, but it's a bit hacky.

// kanel.js
import kanel from 'kanel';

function addExtensionHook(path, lines, instantiatedConfig) {
    return lines.map(line => line.replace(/^import\stype\s.*'(.*)';$/, (match, p1) => match.replace(p1, `${p1}.js`)))
}

await kanel.processDatabase({
    connection: {
        // ...
    },

    postRenderHooks: [addExtensionHook],
});

Essentially what this does is check each line for an import statement, and if it finds one it appends .js to the import path.

I don't think this is a terrible solution overall, but I think it would be easier and more intuitive to have that built-in to the main Config object so it can be set there instead of using a hook.

That's all, cheers!

@kristiandupont
Copy link
Owner

Right, I am semi-aware of this issue. I remember seeing discussions in the TS repo about whether or not you should be able to specify .ts but I guess they decided not to? Should it always say .js even when you are import type'ing, which will be removed in the output?

@virtuallyunknown
Copy link
Author

Hey, thanks for showing interest in this issue. Please be warned that I don't consider myself an expert in this, but my basic understanding is that with CommonJS you can be less strict and leave out the extension when importing. That is when you set moduleResolution to node. From TypeScript 4.7 and onward you get the node16 and nodenext options, which essentially mimics the modern ECMAScript modules implementation in terms of how imports are resolved in TypeScript land.

Relative specifiers like './startup.js' or '../config.mjs'. They refer to a path relative to the location of the importing file. The file extension is always necessary for these.

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

https://nodejs.org/api/esm.html#import-specifiers

Relative import paths need full extensions (we have to write import "./foo.js" instead of import "./foo").

https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#type-in-package-json-and-new-extensions

I am just leaving these out for your convenience if you want to check the docs because as I said I'm not an expert, but I do think you need to specify an extension in all cases.

Unfortunately, I can't categorically answer your question about import type-ing, and I couldn't find much info about it. On paper, it does make sense that you could omit the extension if it's just a type import since they are removed after compiling anyway, but I speculate they decided to keep it as a requirement so it's consistent with normal imports.

So should type imports also have .js? I think so, because I got an error as demonstrated in the screenshot above.

@kristiandupont
Copy link
Owner

Right. I think it will need to support .cjs and .mjs as well. And in this regard, I remember seeing an issue running the CLI when package.json had "type" = "module".

I know that Sindre Sorhus is telling me and everyone else to switch to ESM so I do need to look into this, but there are quite a few things about it that still confuse me..

@virtuallyunknown
Copy link
Author

Right. I think it will need to support .cjs and .mjs as well.

Well, your library (Kanel) only ever outputs type definitions, so personally I can't envision a scenario where it would be necessary to import those as cjs or mjs, I am not totally sure how one would be able to reach that point. Personally, I've never seen that, but that is not to say that such situation is impossible to occur, I'm not sure.

My original idea when submitting this issue was to perhaps have a moduleResolution (could also be named differently) or similar option that you pass to processDatabase, and then for example it would just mimic what ESM and TypeScript do in terms of path resolution. Or alternatively a much simplified version - appendJsToImportPath or something named better, which would just append .js to these paths.

And in this regard, I remember seeing an issue running the CLI when package.json had "type" = "module".

Yep, I only use module these days, and a .kanelrc.js didn't work for me, neither renaming it to .kanelrc.cjs (I think your library just doesn't search for a .cjs file?). But none of that is really an issue (at least for me), because you can just create a normal file, put all your config there and call it via node kanel.js or however else you file is named.


As for the rest of what you said, I can only vouch for ESM. I have some experience with it working with monorepos which are a similar concept to a library, and when I switched to ESM it made a certain things way easier. The two articles I linked above pretty much explain how to use them and most of the details, but perhaps if you have some particular question I might be able to help. Your library doesn't have a lot of dependencies, so I don't think you will have much difficulties if you decided to transition one day.

@kristiandupont
Copy link
Owner

Well, your library (Kanel) only ever outputs type definitions

It can output enums which do create code. I am not sure if it's a problem though. The solution should be pretty easy, the importGenerator simply needs to support extensions, I just want to make sure we cover the necessary cases.

@kristiandupont
Copy link
Owner

So I think you are right about ESM. I don't have any particular questions at the moment because I just have a bit of research to do. I will start looking into it, thank you for nudging me :-)

@virtuallyunknown
Copy link
Author

It can output enums which do create code.

You're right, I totally forgot about enums because I never use them, apologies for that.

So I think you are right about ESM. I don't have any particular questions at the moment because I just have a bit of research to do. I will start looking into it, thank you for nudging me :-)

Hey, no problem. I actually decided to experiment with it a little bit and managed to get a working fork demo, you can find it here:

https://github.com/virtuallyunknown/kanel

You can see all the changes I made here and I made a quick INSTALL.md to test it.

This is by no means ready for production or anything like that, and I haven't researched into CJS backwards compatibility support (if this is something you care about), but other than that I think it works as expected and honestly it only took a couple of hours. Maybe you find this useful, maybe not.

Cheers.

@kristiandupont
Copy link
Owner

Nice work! Just to manage expectations, I will need a bit of time to get comfortable with this and as I am just starting up a new consulting client at the moment, I don't have the excess time and energy right now.

I promise I will look into this as soon as I have a bit of time, I do consider it important.

@virtuallyunknown
Copy link
Author

Take as much time as you want, I am not in a rush about this whatsoever and of course real life takes priority. It was actually you who initially brought up the idea of converting to ESM, I merely suggested that it's great :)

Will be keeping an eye on the repo, cheers!

@thelinuxlich
Copy link
Contributor

I'm facing this issue right now, all my projects use type: "module" in package.json and kanel refuses to run :(

@thelinuxlich
Copy link
Contributor

I was able to get around this issue using the programmatic API, I wonder if it shouldn't be documented as the default way, since ESM users will just grow more and more:

import kk from "kanel-kysely";
const { makeKyselyHook } = kk;
import { recase } from "@kristiandupont/recase";
import kanel from "kanel";
const outputPath = "./models";
const toPascalCase = recase("snake", "pascal");

await kanel.processDatabase({
	connection: {
		host: "localhost",
		user: "postgres",
		password: "postgres",
		database: "postgres",
		port: 5432,
	},
	outputPath,
	schemas: ["public"],
	resolveViews: true,
	preDeleteOutputFolder: true,
	enumStyle: "type",
	preRenderHooks: [makeKyselyHook({ databaseFilename: "Database" })],
	generateIdentifierType: (c, d, config) => {
		const tableName = toPascalCase(d.name);
		const name = toPascalCase(c.name);
		const fullName = tableName + name;
		const innerType = kanel.resolveType(c, d, {
			...config,
			// @ts-ignore
			generateIdentifierType: undefined,
		});
		return {
			declarationType: "typeDeclaration",
			name: fullName,
			exportAs: "named",
			typeDefinition: [`${innerType}`],
			comment: [],
		};
	},
	customTypeMap: {
		"pg_catalog.tsvector": "Set<string>",
		"pg_catalog.int4": "number",
		"pg_catalog.json": "string",
		"pg_catalog.jsonb": "string",
		"pg_catalog.bpchar": "string",
		"public.citext": "string",
	},
});

@virtuallyunknown
Copy link
Author

virtuallyunknown commented Jul 18, 2023

@thelinuxlich how exactly does this solve your problem? Because as far as I'm aware, relative import paths need explicit file extension at their end, otherwise it's not valid ESM, and the code you showed above doesn't address that issue (?). Maybe your tsconfig is different, but I have this:

{
    "compilerOptions": {
        "module": "esnext",
        "moduleResolution": "NodeNext",
        "target": "esnext"
}

and it's not working.

Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './YourImport.js'?

When I looked into this issue some months ago, the only workaround I found was through postRenderHooks where you check if each line is a type import through regular expressions, then you capture (group) the import path itself an append a .js extension.

export function convertESMPaths(path, lines, instantiatedConfig) {
    return lines.map(line => line.replace(/^import\stype\s.*'(.*)';$/, (match, p1) => {
        return /\sfrom\s'kysely';$/.test(match)
            ? match
            : match.replace(p1, `${p1}.js`)
    }))
}

import kanel from 'kanel';
import kanelKysely from 'kanel-kysely';

export async function processDatabase() {
    await kanel.processDatabase({
        connection: {
            // credentials
        },

        preRenderHooks: [kanelKysely.makeKyselyHook()],
        postRenderHooks: [convertESMPaths],
    });
}

await processDatabase();

Needless to say, this is far from an ideal solution.

@thelinuxlich
Copy link
Contributor

Hey @virtuallyunknown , I forgot to say I run this script with tsx, which automaticallly solves lots of ESM problems

@thelinuxlich
Copy link
Contributor

Also sharing my base tsconfig.json for science:

{
  "compilerOptions": {
    "target": "esnext",
    "incremental": true,
    "composite": true,
    "module": "esnext",
    "moduleResolution": "node",
    "inlineSourceMap": true,
    "noEmit": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "exactOptionalPropertyTypes": false,
    "allowUnreachableCode": false,
    "noUncheckedIndexedAccess": true,
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true,
    "preserveSymlinks": false,
    "verbatimModuleSyntax": true
  },
  "include": ["src/**/*.ts"],
  "exclude": ["**/node_modules"]
}

@virtuallyunknown
Copy link
Author

Hey @virtuallyunknown , I forgot to say I run this script with tsx, which automaticallly solves lots of ESM problems

Yep, that explains it, and also you have moduleResolution: "node", if you set it to nodeNext it's going to error out at compile time.

@thelinuxlich
Copy link
Contributor

thelinuxlich commented Jul 18, 2023

Yeah I gave up on moduleResolution: "nodenext" because it doesn't seem to respect allowSyntheticDefaultImports: true

@tzezar
Copy link

tzezar commented Aug 20, 2024

I would appreciate if someone could help me configure this with the tsconfig.json given below. Sadly none of solutions proposed previously works in my case and I am kinda stuck. Thanks in advance!

{
  "compilerOptions": {
    /* Visit https://aka.ms/tsconfig to read more about this file */

    /* Projects */
    // "incremental": true,                              /* Save .tsbuildinfo files to allow for incremental compilation of projects. */
    // "composite": true,                                /* Enable constraints that allow a TypeScript project to be used with project references. */
    // "tsBuildInfoFile": "./.tsbuildinfo",              /* Specify the path to .tsbuildinfo incremental compilation file. */
    // "disableSourceOfProjectReferenceRedirect": true,  /* Disable preferring source files instead of declaration files when referencing composite projects. */
    // "disableSolutionSearching": true,                 /* Opt a project out of multi-project reference checking when editing. */
    // "disableReferencedProjectLoad": true,             /* Reduce the number of projects loaded automatically by TypeScript. */

    /* Language and Environment */
    "target": "ES2022" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
    // "lib": [],                                        /* Specify a set of bundled library declaration files that describe the target runtime environment. */
    // "jsx": "preserve",                                /* Specify what JSX code is generated. */
    // "experimentalDecorators": true,                   /* Enable experimental support for legacy experimental decorators. */
    // "emitDecoratorMetadata": true,                    /* Emit design-type metadata for decorated declarations in source files. */
    // "jsxFactory": "",                                 /* Specify the JSX factory function used when targeting React JSX emit, e.g. 'React.createElement' or 'h'. */
    // "jsxFragmentFactory": "",                         /* Specify the JSX Fragment reference used for fragments when targeting React JSX emit e.g. 'React.Fragment' or 'Fragment'. */
    // "jsxImportSource": "",                            /* Specify module specifier used to import the JSX factory functions when using 'jsx: react-jsx*'. */
    // "reactNamespace": "",                             /* Specify the object invoked for 'createElement'. This only applies when targeting 'react' JSX emit. */
    // "noLib": true,                                    /* Disable including any library files, including the default lib.d.ts. */
    // "useDefineForClassFields": true,                  /* Emit ECMAScript-standard-compliant class fields. */
    // "moduleDetection": "auto",                        /* Control what method is used to detect module-format JS files. */

    /* Modules */
    "module": "NodeNext" /* Specify what module code is generated. */,
    // "rootDir": "./",                                  /* Specify the root folder within your source files. */
    "moduleResolution": "NodeNext" /* Specify how TypeScript looks up a file from a given module specifier. */,
    // "baseUrl": "./",                                  /* Specify the base directory to resolve non-relative module names. */
    "paths": {
      "@database/*": ["./src/database/*"],
      "@utils/*": ["./src/utils/*"],
    },                                      /* Specify a set of entries that re-map imports to additional lookup locations. */
    // "rootDirs": [],                                   /* Allow multiple folders to be treated as one when resolving modules. */
    // "typeRoots": [],                                  /* Specify multiple folders that act like './node_modules/@types'. */
    // "types": [],                                      /* Specify type package names to be included without being referenced in a source file. */
    // "allowUmdGlobalAccess": true,                     /* Allow accessing UMD globals from modules. */
    // "moduleSuffixes": [],                             /* List of file name suffixes to search when resolving a module. */
    "allowImportingTsExtensions": true /* Allow imports to include TypeScript file extensions. Requires '--moduleResolution bundler' and either '--noEmit' or '--emitDeclarationOnly' to be set. */,
    // "resolvePackageJsonExports": true,                /* Use the package.json 'exports' field when resolving package imports. */
    // "resolvePackageJsonImports": true,                /* Use the package.json 'imports' field when resolving imports. */
    // "customConditions": [],                           /* Conditions to set in addition to the resolver-specific defaults when resolving imports. */
    "resolveJsonModule": true /* Enable importing .json files. */,
    // "allowArbitraryExtensions": true,                 /* Enable importing files with any extension, provided a declaration file is present. */
    // "noResolve": true,                                /* Disallow 'import's, 'require's or '<reference>'s from expanding the number of files TypeScript should add to a project. */

    /* JavaScript Support */
    // "allowJs": true,                                  /* Allow JavaScript files to be a part of your program. Use the 'checkJS' option to get errors from these files. */
    // "checkJs": true,                                  /* Enable error reporting in type-checked JavaScript files. */
    // "maxNodeModuleJsDepth": 1,                        /* Specify the maximum folder depth used for checking JavaScript files from 'node_modules'. Only applicable with 'allowJs'. */

    /* Emit */
    // "declaration": true,                              /* Generate .d.ts files from TypeScript and JavaScript files in your project. */
    // "declarationMap": true,                           /* Create sourcemaps for d.ts files. */
    // "emitDeclarationOnly": true,                      /* Only output d.ts files and not JavaScript files. */
    // "sourceMap": true,                                /* Create source map files for emitted JavaScript files. */
    // "inlineSourceMap": true,                          /* Include sourcemap files inside the emitted JavaScript. */
    // "outFile": "./",                                  /* Specify a file that bundles all outputs into one JavaScript file. If 'declaration' is true, also designates a file that bundles all .d.ts output. */
    "outDir": "./dist" /* Specify an output folder for all emitted files. */,
    // "removeComments": true,                           /* Disable emitting comments. */
    "noEmit": true /* Disable emitting files from a compilation. */,
    // "importHelpers": true,                            /* Allow importing helper functions from tslib once per project, instead of including them per-file. */
    // "downlevelIteration": true,                       /* Emit more compliant, but verbose and less performant JavaScript for iteration. */
    // "sourceRoot": "",                                 /* Specify the root path for debuggers to find the reference source code. */
    // "mapRoot": "",                                    /* Specify the location where debugger should locate map files instead of generated locations. */
    // "inlineSources": true,                            /* Include source code in the sourcemaps inside the emitted JavaScript. */
    // "emitBOM": true,                                  /* Emit a UTF-8 Byte Order Mark (BOM) in the beginning of output files. */
    // "newLine": "crlf",                                /* Set the newline character for emitting files. */
    // "stripInternal": true,                            /* Disable emitting declarations that have '@internal' in their JSDoc comments. */
    // "noEmitHelpers": true,                            /* Disable generating custom helper functions like '__extends' in compiled output. */
    // "noEmitOnError": true,                            /* Disable emitting files if any type checking errors are reported. */
    // "preserveConstEnums": true,                       /* Disable erasing 'const enum' declarations in generated code. */
    // "declarationDir": "./",                           /* Specify the output directory for generated declaration files. */

    /* Interop Constraints */
    // "isolatedModules": true,                          /* Ensure that each file can be safely transpiled without relying on other imports. */
    // "verbatimModuleSyntax": true,                     /* Do not transform or elide any imports or exports not marked as type-only, ensuring they are written in the output file's format based on the 'module' setting. */
    // "isolatedDeclarations": true,                     /* Require sufficient annotation on exports so other tools can trivially generate declaration files. */
    // "allowSyntheticDefaultImports": true,             /* Allow 'import x from y' when a module doesn't have a default export. */
    "esModuleInterop": true /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */,
    // "preserveSymlinks": true,                         /* Disable resolving symlinks to their realpath. This correlates to the same flag in node. */
    "forceConsistentCasingInFileNames": true /* Ensure that casing is correct in imports. */,

    /* Type Checking */
    "strict": true /* Enable all strict type-checking options. */,
    // "noImplicitAny": true,                            /* Enable error reporting for expressions and declarations with an implied 'any' type. */
    // "strictNullChecks": true,                         /* When type checking, take into account 'null' and 'undefined'. */
    // "strictFunctionTypes": true,                      /* When assigning functions, check to ensure parameters and the return values are subtype-compatible. */
    // "strictBindCallApply": true,                      /* Check that the arguments for 'bind', 'call', and 'apply' methods match the original function. */
    // "strictPropertyInitialization": true,             /* Check for class properties that are declared but not set in the constructor. */
    // "noImplicitThis": true,                           /* Enable error reporting when 'this' is given the type 'any'. */
    // "useUnknownInCatchVariables": true,               /* Default catch clause variables as 'unknown' instead of 'any'. */
    // "alwaysStrict": true,                             /* Ensure 'use strict' is always emitted. */
    // "noUnusedLocals": true,                           /* Enable error reporting when local variables aren't read. */
    // "noUnusedParameters": true,                       /* Raise an error when a function parameter isn't read. */
    // "exactOptionalPropertyTypes": true,               /* Interpret optional property types as written, rather than adding 'undefined'. */
    // "noImplicitReturns": true,                        /* Enable error reporting for codepaths that do not explicitly return in a function. */
    // "noFallthroughCasesInSwitch": true,               /* Enable error reporting for fallthrough cases in switch statements. */
    // "noUncheckedIndexedAccess": true,                 /* Add 'undefined' to a type when accessed using an index. */
    // "noImplicitOverride": true,                       /* Ensure overriding members in derived classes are marked with an override modifier. */
    // "noPropertyAccessFromIndexSignature": true,       /* Enforces using indexed accessors for keys declared using an indexed type. */
    // "allowUnusedLabels": true,                        /* Disable error reporting for unused labels. */
    // "allowUnreachableCode": true,                     /* Disable error reporting for unreachable code. */

    /* Completeness */
    // "skipDefaultLibCheck": true,                      /* Skip type checking .d.ts files that are included with TypeScript. */
    "skipLibCheck": true /* Skip type checking all .d.ts files. */,
    "noErrorTruncation": true
  },
  "include": ["src/**/*.ts", "backup/drizzle.ts"],
  "exclude": ["node_modules"],
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants