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

some issues with Promise #3

Open
mattdesl opened this issue Apr 29, 2015 · 2 comments
Open

some issues with Promise #3

mattdesl opened this issue Apr 29, 2015 · 2 comments

Comments

@mattdesl
Copy link
Contributor

I think promise is a pretty big section that should probably be split into its own lesson

Some issues I'm seeing:

  • we should encourage reject(new Error('str')) rather than rejecting with strings
  • we should encourage capitalizing Promise constructor
  • Promise.reduce() and catching SyntaxError is not part of the Promise spec; I guess we should decide whether the lesson uses simple ES6 promises, or Bluebird, or a bit of both
  • .then(onResolve, onReject) should be described a bit
  • memoization seems like a bit of an advanced pattern, I think it's generally not needed and might just confuse students as they are trying to learn the basics
  • Promise.resolve, Promise.reject and Promise.all should be described somewhere
  • instead of using reduce the last examples should be using map (reduce is used when you want to resolve to a single value, e.g. getting the max width of all loaded images)

for e.g.

var images = Promise.map(imageArray, loadImage);
images.then(renderImages);

Or with ES6:

var images = Promise.all(imageArray.map(loadImage));

Or with error handling:

var notFoundImage = loadImage('notfound.png')

var images = Promise.map(imageArray, function(file) {
  return loadImage(file)
    .then(null, function(err) {
      return notFoundImage
    })
})
@mattdesl
Copy link
Contributor Author

mattdesl commented Mar 2, 2016

Not sure if these and the other PRs flew under the radar. Should I merge?

@mikkoh
Copy link
Contributor

mikkoh commented Mar 7, 2016

Yeah for some reason I hadn't even seen em. I merged.

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

No branches or pull requests

2 participants