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

oncreate async, fixes #904 #907

Merged
merged 2 commits into from
Oct 28, 2017

Conversation

m59peacemaker
Copy link
Contributor

Just checking if the function is async and outputting async function if so, rather than just always only outputting function. The tests don't do anything but use await inside the function. If the async keyword is not present, using await in the function is a syntax error, so runtime tests will fail.

I also wrote tests for js/samples, but as the README says to avoid those and the runtime tests are sufficient, I am not including them unless you'd like me to.

@m59peacemaker m59peacemaker changed the title Oncreate async, fixes #904 oncreate async, fixes #904 Oct 25, 2017
@m59peacemaker
Copy link
Contributor Author

Should I add the "skip ssr" option to the config for these tests?

@Conduitry
Copy link
Member

I don't think it's the SSR that's the issue, it's the version of Node that's running the tests - version 6 which doesn't support async/await. I wasn't thinking about that complication when I suggested this method for testing... Not sure the best way to go forward here. It'd be nice not to write a js/samples test for this, as those are annoying to maintain. But only running unit tests on more recent versions of Node isn't great either, as there are certainly going to be people running Svelte under Node 4 and Node 6 for quite a while.

@m59peacemaker
Copy link
Contributor Author

Hmm. Well, like I said, I have those js/samples tests ready to go if you decide that's preferable. Otherwise, let me know if I can help some other way to solve this.

@Rich-Harris Rich-Harris merged commit 5275892 into sveltejs:master Oct 28, 2017
@Conduitry
Copy link
Member

c5943d7 Oh heh that's cute, and fairly obvious in hindsight

@Rich-Harris
Copy link
Member

except i messed it up! needed to be [1], not [0]...

@Rich-Harris
Copy link
Member

Right, all the tests are passing now. Thanks @m59peacemaker!

@m59peacemaker
Copy link
Contributor Author

Glad to help!

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.

3 participants