-
Notifications
You must be signed in to change notification settings - Fork 677
Templates SDK JS - Use classes instead of interface #891
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
… template index.ts
…httpx_args; update timestamp logging in AsyncTemplate and Template classes to use datetime.now() instead of datetime.utcnow().
…move unused imports and update test cases to reflect changes.
|
it doesn't have to because base image will always be specified |
| def get_template_base(self) -> "TemplateBase": | ||
| return self.__template |
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.
why do you need this?
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.
because I made template private property, I think it's better this way because you're explicitly have to call the function so you see where the template builder is coming from
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.
Bug: Template Class Undefined, Serialization Validation Missing
The TemplateClass type is undefined, causing compilation errors for static methods. The Template() factory function also returns TemplateBase, which doesn't implement getTemplateBase() as expected by these methods. Additionally, the serialize method no longer validates that a template specifies either fromImage or fromTemplate, which could lead to invalid template data for the build API.
packages/js-sdk/src/template/index.ts#L551-L579
E2B/packages/js-sdk/src/template/index.ts
Lines 551 to 579 in acccb35
| public serialize(steps: Step[]): TriggerBuildTemplate { | |
| const templateData: TemplateType = { | |
| steps, | |
| force: this.force, | |
| } | |
| if (this.baseImage !== undefined) { | |
| templateData.fromImage = this.baseImage | |
| } | |
| if (this.baseTemplate !== undefined) { | |
| templateData.fromTemplate = this.baseTemplate | |
| } | |
| if (this.startCmd !== undefined) { | |
| templateData.startCmd = this.startCmd | |
| } | |
| if (this.readyCmd !== undefined) { | |
| templateData.readyCmd = this.readyCmd | |
| } | |
| return templateData as TriggerBuildTemplate | |
| } | |
| } | |
| // Factory function to create Template instances without 'new' | |
| export function Template(options?: TemplateOptions): TemplateBase { |
packages/js-sdk/src/template/index.ts#L311-L347
E2B/packages/js-sdk/src/template/index.ts
Lines 311 to 347 in acccb35
| static async toJSON(template: TemplateClass): Promise<string> { | |
| const templateBase = template.getTemplateBase() | |
| return JSON.stringify( | |
| templateBase.serialize(await templateBase.calculateFilesHashes()), | |
| undefined, | |
| 2 | |
| ) | |
| } | |
| static toDockerfile(template: TemplateClass): string { | |
| const templateBase = template.getTemplateBase() | |
| if (templateBase.baseTemplate !== undefined) { | |
| throw new Error( | |
| 'Cannot convert template built from another template to Dockerfile. ' + | |
| 'Templates based on other templates can only be built using the E2B API.' | |
| ) | |
| } | |
| if (templateBase.baseImage === undefined) { | |
| throw new Error('No base image specified for template') | |
| } | |
| let dockerfile = `FROM ${templateBase.baseImage}\n` | |
| for (const instruction of templateBase.instructions) { | |
| dockerfile += `${instruction.type} ${instruction.args.join(' ')}\n` | |
| } | |
| if (templateBase.startCmd) { | |
| dockerfile += `ENTRYPOINT ${templateBase.startCmd}\n` | |
| } | |
| return dockerfile | |
| } | |
| static async build( | |
| template: TemplateClass, | |
| options: BuildOptions | |
| ): Promise<void> { |
Change
In JavaScript specifically this is implemented using interfaces unlike classes in Python. I've found some issues with the interfaces-based approach - you're able to pass empty TemplateBase to Template.build when you are not supposed to and also there was also much duplication in types.ts.
It's also probably not safe at all when using pure JS without TypeScript as you won't see compiler errors.
it was working until this point, because we had other methods on the TemplateFinal interface, but it's now empty so it doesn't match anything anymore.