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

feat(resolvers): JavaScript resolvers #534

Merged
merged 8 commits into from
Nov 30, 2022
Merged

feat(resolvers): JavaScript resolvers #534

merged 8 commits into from
Nov 30, 2022

Conversation

bboure
Copy link
Collaborator

@bboure bboure commented Nov 29, 2022

  • Adds support for JS resolvers
  • JS resolvers are now default
  • PIPELINE resolvers are now defaults
  • Removes support for default mapping templates and locations
  • Removes support for named data sources in resolver definition

@bboure bboure marked this pull request as draft November 29, 2022 15:34
@bboure bboure marked this pull request as ready for review November 29, 2022 21:50
@bboure bboure changed the title feat(resolvers): JavaSCript resolvers feat(resolvers): JavaScript resolvers Nov 29, 2022
@bboure bboure merged commit 7f3349d into alpha Nov 30, 2022
@bboure bboure deleted the js-resolvers branch November 30, 2022 20:22
@github-actions
Copy link

🎉 This PR is included in version 2.0.0-alpha.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ebisbe
Copy link
Contributor

ebisbe commented Dec 22, 2022

Is there any difference on how to use substitutions? I don't see anything explicit and I want to try it. My guess would be that they can be used from the env but ....

@ebisbe
Copy link
Contributor

ebisbe commented Dec 22, 2022

Reading the code I see there is no option for that. How could we add vars like substitutions?

@bboure
Copy link
Collaborator Author

bboure commented Dec 22, 2022

This is not supported (yet).
This is more tricky I think than with VTL
Ideally, we'd want ApPSync to support env vars

An alternative would be to use global variables.
Something like

const MY_VAR = "foo";

we could generate those and prepend them to all js files.

in the resolver you could do (fake example)

function request() {
  return {
    "operation": "foo",
    "myVar": MY_VAR,
  }
}

ideas?

@ebisbe
Copy link
Contributor

ebisbe commented Dec 22, 2022

Yes, it's more tricky. I was reviewing the code and saw how it was implemented with VTL. In a way is much more straight forward as it's al string based.

By the way, your suggestion is to use the actual definition:

substitutions:
  ENV: ${self:provider.stage}
  FlickrKey: ${env:FLICKR_KEY, 'FLICKR_KEY env missing'}

and inject them as:

const ENV = "foo";
const FlickrKey = "bar":

I think that it should be something that it is easy to test at the same time.

@ebisbe
Copy link
Contributor

ebisbe commented Dec 22, 2022

Not sure what other things we should consider but something like this?

var fs = require('fs')
var data = fs.readFileSync('User.buddyicon.js', 'utf-8');

const replace = data.replace(/#FlickrKey#/g, '123-abc')

fs.writeFile('User.buddyicon_v2.js', replace, 'utf-8', function (err) {
  if (err) throw err;
})

We define:

const FLICKR_KEY = '#FlickrKey#'

and it's replaced for:

const FLICKR_KEY = '123-abc'

We can't use the same ${} as it's a valid JS syntax so maybe we could use a new one for both VTL and JS. Leaving a deprecation for the existing syntax on the VTL.
Also the proposed solution seems testable as we have pre-filled values to do expectations.

@markgibaud-vtail
Copy link

Was there an explanation for removing support for default mapping templates and locations? Can defaults be implemented in a different way or is it a case of not doing AppSync AWS's way?

@bboure
Copy link
Collaborator Author

bboure commented Jan 12, 2023

@markgibaud-vtail

Yes, it's explained here

More context: with JS resolvers, it can cause confusion and unexpected results.
Like: What is the default? VTL resolvers? or JS?
I also did not want to introduce a runtime option because it is already implicit from code (JS) or request/response (VTL). Requiring both would be repetitive.

I thought it would be better if the developer has to be explicit about it.

Another reason is that, when you look at the Serverless Framework, the is no default for the Lambda functions' handler, right? You MUST provide the location of your handler. This plugin should not be different.

In V2, I am trying to be more in line with SF's core behaviors.

@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

vicary pushed a commit to vicary/serverless-appsync-plugin that referenced this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants