-
Notifications
You must be signed in to change notification settings - Fork 285
feat: enhance file resolution logic for Convex #127
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
base: main
Are you sure you want to change the base?
Conversation
…ma files - Updated bundler to check for multiple file extensions (.ts, .js, .mjs, .cjs) for schema and auth configuration files. - Improved component root path resolution to prioritize new file types. - Adjusted comments and error messages for clarity regarding supported file types. This change ensures better compatibility and flexibility in handling various configuration file formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want the component-related changes, but if anyone has a reason to need to use alternative file extensions for files like schema and auth.config we can do that; just want to hear about the uses cases first so we can add realistic tests of them and we know what we're supporting
let target = path.resolve(dir, "schema.ts"); | ||
if (!ctx.fs.exists(target)) { | ||
target = path.resolve(dir, "schema.js"); | ||
const candidates = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would love to hear if people are using these extensions and why, but if this is helpful we can make this change. I'd guess the various TypeScript extensions would be more important? .mts, .cts, etc than .js, since most users use TypeScript? But I haven't heard requests for either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt see a lot of developer using .mts and .cts for typescript files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine if we remove .cjs, we never want to bundle a commonjs schema.
@@ -54,19 +54,27 @@ import { Reporter, Span } from "./tracing.js"; | |||
import { | |||
DEFINITION_FILENAME_JS, | |||
DEFINITION_FILENAME_TS, | |||
DEFINITION_FILENAME_MJS, | |||
DEFINITION_FILENAME_CJS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the component changes I do not want to make without understanding the use cases. The spec for components isn't well-defined yet but I wouldn't want to make what's accepted anyy more broad than necessary without specifics use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a response in #126 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want the CJS one. I don't know that we really need a .mjs extension either, but if this is a difficult limitation for a packaging tool we can add .mjs if we also add a component that works like this to make sure we don't regress the behavior.
If you'd like to use packem to build convex components, would you consider adding the ability to produce .js files for ESM-only builds of a package with "type": "module"
? e.g. with tsdown
, an ESM-only build configured like below produces ESM files with .js extensions. Component definitions are already always ESM, so it's never necessary to distinguish between these.
import { defineConfig } from "tsdown";
export default [
defineConfig({
entry: ["src/index.ts", "src/another.ts"],
format: ["cjs", "esm"],
dts: {
tsconfig: "./tsconfig.json",
sourcemap: true,
},
}),
defineConfig({
entry: ["src/server.ts"],
format: ["esm"],
dts: {
tsconfig: "./tsconfig.json",
sourcemap: true,
},
}),
];
let target = path.resolve(dir, "schema.ts"); | ||
if (!ctx.fs.exists(target)) { | ||
target = path.resolve(dir, "schema.js"); | ||
const candidates = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine if we remove .cjs, we never want to bundle a commonjs schema.
const authConfigPath = path.resolve(dir, "auth.config.js"); | ||
const authConfigTsPath = path.resolve(dir, "auth.config.ts"); | ||
if (ctx.fs.exists(authConfigPath) && ctx.fs.exists(authConfigTsPath)) { | ||
const candidates = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth.config.ts doesn't exist in components so there's no need to change this
@@ -54,19 +54,27 @@ import { Reporter, Span } from "./tracing.js"; | |||
import { | |||
DEFINITION_FILENAME_JS, | |||
DEFINITION_FILENAME_TS, | |||
DEFINITION_FILENAME_MJS, | |||
DEFINITION_FILENAME_CJS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want the CJS one. I don't know that we really need a .mjs extension either, but if this is a difficult limitation for a packaging tool we can add .mjs if we also add a component that works like this to make sure we don't regress the behavior.
If you'd like to use packem to build convex components, would you consider adding the ability to produce .js files for ESM-only builds of a package with "type": "module"
? e.g. with tsdown
, an ESM-only build configured like below produces ESM files with .js extensions. Component definitions are already always ESM, so it's never necessary to distinguish between these.
import { defineConfig } from "tsdown";
export default [
defineConfig({
entry: ["src/index.ts", "src/another.ts"],
format: ["cjs", "esm"],
dts: {
tsconfig: "./tsconfig.json",
sourcemap: true,
},
}),
defineConfig({
entry: ["src/server.ts"],
format: ["esm"],
dts: {
tsconfig: "./tsconfig.json",
sourcemap: true,
},
}),
];
return componentRootPath; | ||
|
||
// Default fallback to .ts for backward compatibility | ||
return path.resolve(path.join(functionsDir, DEFINITION_FILENAME_TS)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a behavior change, previously the default was the JS path
@@ -53,7 +53,7 @@ function componentPlugin({ | |||
name: `convex-${mode === "discover" ? "discover-components" : "bundle-components"}`, | |||
async setup(build) { | |||
// This regex can't be really precise since developers could import | |||
// "convex.config", "convex.config.js", "convex.config.ts", etc. | |||
// "convex.config", "convex.config.mjs", "convex.config.cjs", "convex.config.js", "convex.config.ts", etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, throughout the PR remove .cjs
…port - Removed support for .cjs file extension in various components, including bundler and component definitions. - Updated logic to prioritize .js and .ts extensions for configuration files. - Adjusted comments and error messages for clarity regarding supported file types.
- Adjusted the priority order for schema and definition file checks to prioritize .ts over .mjs and .js. - Removed references to .d.cts files in the DTS file collection logic to streamline the process. - Updated comments for clarity regarding the new priority order.
So i did remove the support for the .cjs and .d.cts, and reverted the order of finding back to "ts", "mjs" "js". :) It still could be that a .js is a commonjs file. But there is not easy way to check if a file is esm only (i dont know one) |
configuration and schema files
This change ensures better compatibility and flexibility in handling various configuration file formats.
fixes #126
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.