Adding more auth conversion templates #167
Conversation
This adds auth conversion templates for api keys (header + query string), session, and unknown auths. It also improves Basic and OAuth conversion templates. It also improves and adds tests for conversion.
Specifically for OAuth2, OAuth2 w/ Refresh, Session Auth, and API Key (Header and Query). Also tweaking more tests.
BrunoBernardino
requested review from
eliangcs
and removed request for
bcooksey
|
@bcooksey / @bryanhelmig Not a requirement, but feel free to give me your review on this PR! @eliangcs let me know if you've got any questions on this. The numbers look scary but that's mostly a few |
BrunoBernardino
changed the title
There was a problem hiding this comment.
Awesome work! Covers a lot of things that were ignored. I pulled and tested with a couple of WB apps, and didn't find any major problems. We could have a more complete test before we merge it, but I don't think that's necessary as zapier convert is quite minimal, to begin with, so I think we can be pretty receptive to any improvements. I just have some questions on the package.json changes and a bug on the redundant comma generated to index.js.
| "watch": "rm -rf lib && node_modules/babel-cli/bin/babel.js --watch src -d lib", | ||
| "lint": "node_modules/.bin/eslint zapier.js src test", | ||
| "build": "rm -rf lib && node_modules/babel-cli/bin/babel.js src -d lib --copy-files", | ||
| "watch": "rm -rf lib && node_modules/babel-cli/bin/babel.js --watch src -d lib --copy-files", |
There was a problem hiding this comment.
Just want to clarify. What does --copy-files option do? I compared the results with or without --copy-files. They look the same to me.
There was a problem hiding this comment.
It copies the definitions/*.json files inside src/tests/utils, otherwise only the .js files are passed along, at least for me.
| "lint": "node_modules/.bin/eslint zapier.js src test", | ||
| "build": "rm -rf lib && node_modules/babel-cli/bin/babel.js src -d lib --copy-files", | ||
| "watch": "rm -rf lib && node_modules/babel-cli/bin/babel.js --watch src -d lib --copy-files", | ||
| "lint": "node_modules/.bin/eslint zapier.js src", |
There was a problem hiding this comment.
Any reasons why we don't want to lint test code?
There was a problem hiding this comment.
We're still linting the test code, since it's inside src/tests/*, this wasn't doing anything as there's no /test directory.
src/utils/convert.js
Outdated
|
|
||
| templateContext.REQUIRES = importLines.join('\n'); | ||
| lines.push(`[${varName}.key]: ${varName},`); |
There was a problem hiding this comment.
I think this adds a redundant comma at the end of each line.
There was a problem hiding this comment.
You're right! Nice catch!
This basically adds more auth conversion templates, and a
scripting.jsfile.Relevant Trello Card.
What this changes
Log
Adding more auth conversion templates (fe1f44a)
This adds auth conversion templates for api keys (header + query string), session, and unknown auths.
It also improves Basic and OAuth conversion templates.
It also improves and adds tests for conversion.
Marking clearer to-dos (dbb7521)
Adding more auth conversions and tests (0d5f86b)
Specifically for OAuth2, OAuth2 w/ Refresh, Session Auth, and API Key (Header and Query).
Also tweaking more tests.
Adding scripting generation and package.json improvements. (139f339)
Switching version. (b135bb5)
Skipping test that's killing Travis (df03a42)
Fixing redundant comma for convert on index (ce50077)
Renaming from interpreter to legacy-scripting-runner (1cf7e7b)