Skip to content

Conversation

@benjamingr
Copy link
Contributor

Description

Removes explicit construction for promise code.

Motivation and Context

It's cleaner design.

How Has This Been Tested?

I did not test this at all but I've literally refactored code this way a thousand times.

Checklist:

  • node_modules/.bin/jscs -c .jscsrc stacktrace.js passes without errors
  • npm test passes without errors
  • I have read the contribution guidelines
  • I have updated the documentation accordingly
  • I have added tests to cover my changes

The promise usage left some to be desired and I felt like I should help by refactoring out explicit construction which I used to do myself when I started with promises.

Thanks for the library.

The promise usage left some to be desired and I felt like I should help by refactoring out [explicit construction](http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it/23803744#23803744) which I used to do myself when I started with promises.

Thanks for the library.
@oliversalzburg
Copy link
Member

This is very likely cool, but travis-pr should really be fixed :P

@oliversalzburg oliversalzburg merged commit 11df110 into stacktracejs:master Jun 5, 2016
@eriwen
Copy link
Member

eriwen commented Jun 5, 2016

Thanks for cleaning this up @benjamingr.

@oliversalzburg looks like the failure is legitimate. The use of catch as it stands will break old IE, so we'll need to fix that.

benjamingr added a commit to benjamingr/stacktrace.js that referenced this pull request Jun 5, 2016
I caused a compatibility issue in stacktracejs#166 
since reserved keywords can't be property names in ES3.

This fixes it.
@benjamingr
Copy link
Contributor Author

Sorry @eriwen it's been a while since I worked with code that needs to support IE < 9 without a .caught alias, followed up with a PR to fix that #167

@eriwen
Copy link
Member

eriwen commented Jun 5, 2016

@benjamingr no worries at all. Much respect for following up and fixing it.

@eriwen
Copy link
Member

eriwen commented Jun 6, 2016

Apologies @benjamingr and @oliversalzburg - I'm going to have to revert this for now.

It's not immediately clear to me why this error is thrown when calling fromError:

TypeError: undefined is not a constructor (evaluating 'stackframes.map(function(sf) {
                return gps.pinpoint(sf);
            }).catch(function(error) {
                return sf;
            })') in stacktracejs/stacktrace.js/stacktrace.js (line 89)

Furthermore, this changes behavior in 2 ways:

  • An un-parseable Error no longer rejects properly
  • Fallback behavior when gps.pinpoint fails is not maintained

I'm willing to consider PRs to achieve a cleaner implementation in the future as long as they pass the tests.

eriwen added a commit that referenced this pull request Jun 6, 2016
This reverts commit 11df110, reversing
changes made to b4077c1.
@oliversalzburg
Copy link
Member

My bad :( The error The command "if [ "${TRAVIS_PULL_REQUEST}" = "false" ]; then gulp ci; else gulp pr; fi" exited with 1. was making me think that there was something wrong with the setup.

@eriwen
Copy link
Member

eriwen commented Jun 6, 2016

@oliversalzburg There have been a lot of problems historically with PR builds, so I empathize. I have put a (annoyingly) lot of effort into PR builds, so they should be stable and useful from now on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants