-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat: Add bundling and TS support #576
Conversation
src/resources/JsResolver.ts
Outdated
bundle: true, | ||
write: false, | ||
external: ['@aws-appsync/utils'], | ||
format: 'esm', |
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.
@bboure Thanks for sharing. Love it! I didn't know getting TS support + bundling was that simple. 🚀
I'm now using something similar in one of my CDK projects. However, I found one case where the resulting JS code didn't work. Here's a simplified version of the resolver:
import { util } from '@aws-appsync/utils'
import type { Context } from '@aws-appsync/utils'
export function response(ctx: Context) {
const { result, error } = ctx
if (error) {
return util.error(error.message, error.type)
}
if (result.items.length === 0) {
return util.error('user not found')
}
const user = result.items[0]
return {
id: user.UserID,
slack: {
enabled: user.Slack?.Enabled ?? false,
webhookUrl: user.Slack?.WebhookURL,
channel: user.Slack?.Channel,
},
}
}
Transpiled to JS with esbuild:
// esbuild --bundle --external:@aws-appsync/utils --format=esm resolvers/getUser.ts
import { util } from "@aws-appsync/utils";
function response(ctx) {
var _a, _b, _c, _d;
const { result, error } = ctx;
if (error) {
return util.error(error.message, error.type);
}
if (result.items.length === 0) {
return util.error("user not found");
}
const user = result.items[0];
return {
id: user.UserID,
slack: {
enabled: (_b = (_a = user.Slack) == null ? void 0 : _a.Enabled) != null ? _b : false,
webhookUrl: (_c = user.Slack) == null ? void 0 : _c.WebhookURL,
channel: (_d = user.Slack) == null ? void 0 : _d.Channel
}
};
}
export {
response
};
However, at runtime, the resolver will return ReferenceError: _a is not defined
.
I don't know why that is tbh. The fix was to target ES2020, which introduced optional chaining:
// esbuild --bundle --external:@aws-appsync/utils --format=esm --target=es2020 resolvers/getUser.ts
import { util } from "@aws-appsync/utils";
function response(ctx) {
const { result, error } = ctx;
if (error) {
return util.error(error.message, error.type);
}
if (result.items.length === 0) {
return util.error("user not found");
}
const user = result.items[0];
return {
id: user.UserID,
slack: {
enabled: user.Slack?.Enabled ?? false,
webhookUrl: user.Slack?.WebhookURL,
channel: user.Slack?.Channel
}
};
}
export {
response
};
This resolver works just fine.
So you might consider passing target: 'es2020'
too.
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.
Thanks!
That's super useful! I definitely need to investigate more and do more testing.
In my research, I noticed I needed at least to target es2018. For some reason, I omitted this here.
This is super cool, I'm trying to avoid having 2 separate build process for building the pipelineFunctions. Serverless (with the right config) will transpile our TS resolvers (i.e our custom authorizer in the general "functions" section of serverless config) but not the typescript functions within appSync pipelineFunctions section. If there's anything I can do to help with this PR rather than going down my own path please share. |
I took some time to polish this PR based on the official doc guidelines. If anyone is interested in testing this before we ship it 🚀, it would be much appreciated. clone this repo, then
in your project's folder
|
I can test it. |
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 CloudFormation Resource to generate the appsync definition is in that PR? I don't see the code.
Also the ctx.indentity.claims
is not properly defined. I see that ctx.identity
is from the type Identity and that is an OR of different types which one is AppSyncIdentityOIDC
that includes the claims but it says it's missing.
doc/general-config.md
Outdated
|
||
## Esbuild | ||
|
||
By default, this plugin uses esbuild in order to bundle Javascript resolvers. TypeSCript fiels are also transpiled into compatible JavaScript. This option allows you to pass custom options that must be passed to the esbuild command. |
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.
Typo: TypeSCript fiels
=> TypeScript fields
Also I think that the checkout of this branch has to be done before running
And had to remove / upgrade some packages that are not up to date with TS 5. So if the code is TS 4 compatible then our live will be easier. |
Not sure I understood the question. This PR is changing the following:
I think you are referring to the Context type that you can include from |
ok, sorry as I didn't explain me well enough. I meant having types for the appsync configuration defined in this package. It's pretty complex and can get complex. So having types to define the configuration would be really good. It's probably for a different PR reading all the context of this one. |
It works but some parts are missing. All this issues come from my configuration that is currently working so it should be valid.
That's all I found but looking awesome! |
For me the esbuild configuration is not working. The bundle resolver ends up like: |
src/resources/JsResolver.ts
Outdated
sourcemap: 'inline', | ||
treeShaking: true, | ||
minifyWhitespace: true, | ||
minifyIdentifiers: true, |
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.
minifyIdentifiers: true, | |
minifyIdentifiers: false, |
Otherwise it converts: import{runtime}from\"@aws-appsync/utils\";
into import{runtime as $}from\"@aws-appsync/utils\";
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.
Do you have a way to reproduce this?
In my case it is transpiled to
import{util as e,runtime as r}from\"@aws-appsync/utils\";
(from this)
import {
Context,
DynamoDBGetItemRequest,
util,
runtime,
} from '@aws-appsync/utils';
and it's working as expected at runtime.
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.
Sorry. You are right. I had other issues that the eslint appsync configuration was not caching and I thought it was that.
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.
OK, it seems like this sometimes causes issues after all.
Someone recently asked me for help with an issue on AppSync, using esbuild.
He hit an Runtime error
without any explanation from AppSync.
After some debugging, we figured out he used the --minify-identifiers
option with esbuild which was renaming a variable to _
.
That variable was then passed into a custom function as an argument.
For some reason, AppSync does not like that.
Debugging in the appsync code evaluator gives this error:
Lexical error, Encountered: "_" (95), after : "." at *unset*[line 1, column 876]
example code
const { foo: _ } = ctx.args;
const r = test(_)
It sounds like a rare use case but we might need to consider removing any minification for now (minifyWhitespace
might be safe to keep though) to avoid any issue.
The official AppSync doc does not include modification in their guides.
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 can leave it as opt-in though thought the esbuild
config.
We could even consider allowing different esbuild
options per resolver/funciton to allow fine-tuning each one and work around those kind of issues when they are found
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'm not sure minifying make sense anyway. Is there a limit on each function size? Otherwise I would rather be able to read the whole request/response in case I need what has been deployed
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.
There is a limit.
4kb for inline code (which is what this plugin does at the moment), or 10kb if uploaded to S3.
I've had feedback of people hitting the 4kb limit 😄
🎉 This PR is included in version 2.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.