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

Upgrade http client #260

Merged
merged 8 commits into from
Oct 9, 2020
Merged

Upgrade http client #260

merged 8 commits into from
Oct 9, 2020

Conversation

brandondoran
Copy link
Collaborator

@brandondoran brandondoran commented Oct 7, 2020

Overview

This upgrades the http client and related code for handling the file uploads. To the end users there should be no noticeable change in functionality.

  • Replaces deprecated request dependency with node-fetch.
  • Switch to use tapPromise hook instead of tapAsync. This will allow using async/await or promises instead of callbacks
  • Remove async dependency and convert methods to async/await instead of callbacks.
  • Update tests to work with async/await instead of callbacks
  • Remove lodash.reduce, lodash.find and use native array methods instead.
  • Introduce optional chaining and null coalescing support

🛠️ Testing

Setup

Use react example to serve as an integration test of sorts. The example uses .env to set env vars needed for S3 and Rollbar. Create .env from example and fill it with your own settings:

cp examples/react/.env.example examples/react/.env

To simplify setup, comment out S3Plugin instances in examples/react/webpack.config.js so you don't need AWS creds.

Test steps

  • rm -rf node_modules && npm i
  • build the plugin: npm run build
  • build react example:
    cd examples/react
    npm i
    npm run build
    
  • this will build the app, upload 2 js assets to s3 and 2 sourcemaps to Rollbar
    98% after emitting RollbarSourceMapPlugin
    Uploaded app-8231cb50df131c68dc45.js.map to Rollbar
    Uploaded vendors~app-ab928b5b25b3b2eff91b.js.map to Rollbar
    <w> [webpack.Progress] 2619ms after emitting
    

@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #260 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #260   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           95        87    -8     
  Branches        30        25    -5     
=========================================
- Hits            95        87    -8     
Impacted Files Coverage Δ
src/RollbarSourceMapPlugin.js 100.00% <100.00%> (ø)
src/helpers.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cc7c2b...3384d48. Read the comment docs.

Minimal code changes to migrate away from request module and callback
apis.

* Replaces request module with node-fetch.
* BREAKING CHANGE: drop support for Tapable.plugin. Plugini will no
longer work in webpack v3. >= v4 will be required.
* Use Tapable.hooks.tapPromise instead of tapAsync
* Convert code to use async/await instead of callbacks
* Update tests to reflect async methods as opposed to callbacks
@brandondoran brandondoran merged commit f663b23 into master Oct 9, 2020
@brandondoran brandondoran deleted the http-upgrade branch October 9, 2020 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants