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

refactored app-server internals #54

Closed
wants to merge 11 commits into from
Closed

refactored app-server internals #54

wants to merge 11 commits into from

Conversation

mreinstein
Copy link
Contributor

@mreinstein mreinstein commented Feb 1, 2017

this shouldn't change the public api at all.

fixes #52 & #21, and partially solves #35

I've deployed this to a test amazon skill and verified that at least the hello_world skill works.

this breaks several tests though. unfortunately I don't have time to update the tests and do the administrative work on this right now. sending this PR so others can review, and pick up where I left off.

Besides the tests and administrative duties, this should be good to go.

@mreinstein
Copy link
Contributor Author

@mreinstein
Copy link
Contributor Author

also important to note: this PR depends on this landing first: alexa-js/alexa-app#144

@dblock
Copy link
Collaborator

dblock commented Feb 1, 2017

I merged #144, I'll try to find time for it, including proper CHANGELOG and UPGRADING from alexa-js/alexa-app#144. Anyone please feel free to pick this up and beat me to it.

@rickwargo
Copy link
Collaborator

rickwargo commented Feb 1, 2017

@mreinstein I'll review by tomorrow.

@alexa-app-bot
Copy link

alexa-app-bot commented Feb 1, 2017

Warnings
⚠️ Did you forget to update CHANGELOG.md?

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 91.875% when pulling 1f1d3e5 on mreinstein:master into c92fbc3 on alexa-js:master.

@@ -19,8 +19,7 @@
},
"license": "MIT",
"dependencies": {
"alexa-app": "^2.4.0",
"alexa-verifier-middleware": "^0.1.8",
"alexa-app": "git+https://github.com/mreinstein/alexa-app.git#352f17cf9b8f541d8f32a385bfff61877b60134e",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do this, at the very least point it to "alexa-js/alexa-app".

Copy link
Contributor Author

@mreinstein mreinstein Feb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was a necessary evil while developing/testing. My hosting environment caches deps aggressively. This absolutely should point at canonical npm version as soon as possible. same goes for entries in examples/apps/*package.json

@cpup22
Copy link

cpup22 commented Feb 1, 2017

Patiently waiting for this to get pulled in. I have a skill under awaiting certification that I know is going to fail because of the verification piece is not properly working. Hoping to swap this out on my backend before they get to that test (a.k.a. before I fail certification) so I don't have to wait another week. So... let me know if you all need help testing this. ;)

@dblock
Copy link
Collaborator

dblock commented Feb 2, 2017

I have a merged version in #56, with a bunch of failing tests. If anyone has quick test fixes, add a comment to that - I'll take a look soon.

@tejashah88
Copy link
Member

Closing this PR in favor of #56

@tejashah88 tejashah88 closed this Feb 2, 2017
@dblock
Copy link
Collaborator

dblock commented Feb 3, 2017

This PR breaks the debugger completely. The test failures are legit, see #56 for some of the fixes. I was able to fix template rendering, but not POST of actual intent data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting "verify: true" is causing alexa app to timeout from echo devices
7 participants