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

fix(webpack): provide global URL and URLSearchParams #6864

Merged
merged 6 commits into from
Jan 19, 2020
Merged

fix(webpack): provide global URL and URLSearchParams #6864

merged 6 commits into from
Jan 19, 2020

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Jan 13, 2020

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Fixes #4914 by using built-in url package. Tested against node 8.x too. If we want even more consistency, can switch to https://www.npmjs.com/package/whatwg-url.

Related PRs: vuejs/vue#10414.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@pi0 pi0 requested review from clarkdo, atinux and a team January 13, 2020 14:34
@pi0 pi0 assigned clarkdo and pi0 Jan 13, 2020
@codecov-io
Copy link

codecov-io commented Jan 13, 2020

Codecov Report

Merging #6864 into dev will decrease coverage by 0.05%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6864      +/-   ##
==========================================
- Coverage   64.75%   64.69%   -0.06%     
==========================================
  Files          78       78              
  Lines        2721     2725       +4     
  Branches      708      709       +1     
==========================================
+ Hits         1762     1763       +1     
- Misses        730      732       +2     
- Partials      229      230       +1
Flag Coverage Δ
#unit 64.69% <50%> (-0.06%) ⬇️
Impacted Files Coverage Δ
packages/config/src/config/build.js 100% <ø> (ø) ⬆️
packages/webpack/src/config/server.js 0% <0%> (ø) ⬆️
packages/cli/src/command.js 99.01% <100%> (ø) ⬆️
packages/cli/src/utils/config.js 96.66% <100%> (ø) ⬆️

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 90e4928...fccda75. Read the comment docs.

atinux
atinux previously approved these changes Jan 15, 2020
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

As ProvidePlugin can be easily configured by build.extend, should we provide this special config only for URL polyfill?

@pi0
Copy link
Member Author

pi0 commented Jan 18, 2020

@clarkdo I think so because currently it is broken (#4914). If we do a polyfill, it makes sense to also allow disabling/adjusting this fix (some reported only whatwg-url works). And BTW this option is not documented.

@dhritzkiv
Copy link

dhritzkiv commented Mar 24, 2020

Is there a trick to having this work?

I have a universal app that under v2.11.0 I was able to get URL working in client and SSR by setting config.render.bundleRenderer.runInNewContext to false.

Updating to 2.12.0 results in URL is not a constructor, no matter if I remove that setting or not. Is there something else I need to enable/disable or install in order to have URL work in both SSR and client? Running node 13, btw.

@pi0
Copy link
Member Author

pi0 commented Mar 24, 2020

@dhritzkiv ProvidePlugin is a webpack feature and indeed yes the reason is runInNewContext is enabled by default in dev mode. How sad it broke your project :( Would you please help reproducing in a simple repo to quickly investigate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global.URL is missing
6 participants