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

Check if captureStackTrace exists before calling #288

Merged
merged 7 commits into from
Sep 26, 2018
Merged

Check if captureStackTrace exists before calling #288

merged 7 commits into from
Sep 26, 2018

Conversation

pcvonz
Copy link
Contributor

@pcvonz pcvonz commented Sep 18, 2018

This PR is a:

[ ] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[x] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Check if Error.captureStackTrace exists before calling it.
When this pull request is merged, it will...

Additional information

Fixes #287

@coveralls
Copy link

coveralls commented Sep 18, 2018

Pull Request Test Coverage Report for Build 994

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 32.051%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/peregrine/src/RestApi/Magento2/M2ApiResponseError.js 0 2 0.0%
Totals Coverage Status
Change from base Build 982: -0.04%
Covered Lines: 522
Relevant Lines: 1653

💛 - Coveralls

@zetlen
Copy link
Contributor

zetlen commented Sep 18, 2018

Hey @pcvonz, thanks for catching this. Would you mind adding a test for it, so we don't lose coverage? It would look something like this, in packages/peregrine/src/RestApi/Magento2/__tests__/M2ApiResponseError.spec.js:

test('does not call Error.captureStackTrace if unavailable', () => {
  const capture = Error.captureStackTrace;
  Error.captureStackTrace = null;
  // make sure you can still create an M2ApiResponseError without throwing
  Error.captureStackTrace = capture;
});

@PWAStudioBot
Copy link
Contributor

Fails
🚫

The following file(s) were not formatted with prettier. Make sure to execute npm run prettier locally prior to committing.

packages/peregrine/src/RestApi/Magento2/__tests__/M2ApiResponseError.spec.js

Generated by 🚫 dangerJS

@zetlen zetlen self-requested a review September 26, 2018 15:18
Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

Totally good to merge. Thank you.

@zetlen zetlen merged commit 93b411e into magento:master Sep 26, 2018
zetlen added a commit that referenced this pull request Sep 27, 2018
* Check if captureStackTrace exists before calling (#288)

* Check if captureStackTrace exists before calling

* Add test to ensure captureStackTrace isn't called when it does not exist

* Prettier ✨

* Fix failing test (#304)

Unit tests should use MemoryRouter, not BrowserRouter, and do
shallow rendering rather than full mounting.

* feat(dev): Make PWADevServer host/SSL optional (#175)

PWADevServer attempts to create a unique domain name, create local
SSL certificates, and tell the OS to trust those certificates, for
every new project. It also tries to confirm they exist on every run!

This causes many problems for users under some common conditions:
- No administrative access to local machine
- No OpenSSL installed, or wrong OpenSSL installed
- OS cannot be scripted to trust certificates
- Developer uses Firefox, which uses its own cert store

Additionally, some bugs in the implementation have caused some
developers' projects to enter an unusable state.

- Adds `provideUniqueHost` flag to PWADevServer configuration.
 PWADevServer will no longer try to create or retrieve a custom domain
 name unless `provideUniqueHost` is in its configuration in
 `webpack.config.js` as either a custom string or `true`.
- Adds `provideSSLCert` flag to PWADevServer configuration. PWADevServer
will no longer try to create or retrieve a trusted SSL certificate
unless `provideSSLCert: true` is in its configuration in
`webpack.config.js`.
- Modifies custom domain name creation strategy to ensure uniqueness
based on a hash of the full local path, rather than using the local
flat file database.

We created these features for the needs of the developer working on
several PWAs at once on their local machine, so that they don't have to
set up manual SSL every time, and they have no conflicts with Service
Workers. This could be considered "bonus functionality", as it's not
critical to the setup of a minimum viable PWA. It was meant to establish
our focus on developer experience, and articulate the parts of developer
setup that PWA Studio can "own".

*However, we soon learned that we could not maintain all scenarios for
automated setup and continue to make progress with shopper-facing
features*. We still really want to support and automate all of these
scenarios, but for now, our implementations are a hindrance and we are
turning them off by default.

fixup: Documentation edits from PR feedback

* chore: manually set version 1.1.0 and lerna fix (#305)
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.

4 participants