Skip to content

fix(webpack): fix consola IE compatibility #6298

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

Merged
merged 5 commits into from
Sep 29, 2019
Merged

fix(webpack): fix consola IE compatibility #6298

merged 5 commits into from
Sep 29, 2019

Conversation

clarkdo
Copy link
Member

@clarkdo clarkdo commented Aug 25, 2019

See #5743

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

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.

Sorry, something went wrong.

@clarkdo clarkdo requested a review from pi0 August 25, 2019 10:59
@codecov-io
Copy link

codecov-io commented Aug 25, 2019

Codecov Report

Merging #6298 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #6298   +/-   ##
=======================================
  Coverage   95.61%   95.61%           
=======================================
  Files          79       79           
  Lines        2693     2693           
  Branches      697      697           
=======================================
  Hits         2575     2575           
  Misses        101      101           
  Partials       17       17
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.64% <ø> (ø) ⬆️
#unit 92.31% <ø> (ø) ⬆️
Impacted Files Coverage Δ
packages/webpack/src/config/base.js 94.68% <ø> (ø) ⬆️

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 95e043f...0e34018. Read the comment docs.

@clarkdo clarkdo changed the title feat: build consola from src feat(webpack): build consola from src Aug 25, 2019
@@ -215,7 +216,7 @@ export default class WebpackBaseConfig {
alias () {
return {
...this.buildContext.options.alias,
consola: require.resolve(`consola/dist/consola${this.isServer ? '' : '.browser'}.js`)
consola: require.resolve(`consola/${this.isServer ? 'dist/consola' : 'src/browser'}.js`)
Copy link
Member Author

@clarkdo clarkdo Aug 25, 2019

Choose a reason for hiding this comment

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

Because consola server build will bundle dependencies like dayjs which are not in deps of Nuxt, so use dist/consola.js for server build.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@clarkdo
Copy link
Member Author

clarkdo commented Sep 22, 2019

@pi0

galvez
galvez previously approved these changes Sep 27, 2019
Copy link

@galvez galvez left a comment

Choose a reason for hiding this comment

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

LGTM

TheAlexLichter
TheAlexLichter previously approved these changes Sep 28, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pi0 pi0 dismissed stale reviews from TheAlexLichter and galvez via 55945d9 September 29, 2019 08:51
@pi0
Copy link
Member

pi0 commented Sep 29, 2019

An overall description of different methods to resolve this issue and why going with method of PR.

Problem: We use consola only for development mode. The browser dist use some features that IE9 does not support that makes debugging impossible during dev for IE.

Solutions:

  1. Include babel-polyfill in browser build (fix: include polyfills for browser build unjs/consola#67)
  2. Build from source (this PR)

As consola is only used for dev for browser, our main priority is best DX (not best perf for end-users) and best DX is fastest nuxt dev. Pretranspilation usually increases build time (like BootstrapVue) but for consola, it seems not. Polyfills to the dist add ~22kB to the bundle size which seems to make webpack even slower!


Silly benchmark:

Src: 3.95 / 3.67 / 3.37 / 3.49
Dist with polyfill: 4.6 / 4.06 / 3.9 / 4.13 / 3.84 / 3.82

@pi0 pi0 changed the title feat(webpack): build consola from src fix(webpack): fix consola IE compatibility Sep 29, 2019
@pi0 pi0 merged commit d1df5c8 into dev Sep 29, 2019
@pi0 pi0 deleted the consola-src branch September 29, 2019 09:02
@AndrewBogdanovTSS
Copy link

@pi0 will it affect production build size in any way or it only increases dev bundle?

@pi0
Copy link
Member

pi0 commented Sep 29, 2019

@AndrewBogdanovTSS no it is dev only. And no effect on dev bundle as we had it as before.

@AndrewBogdanovTSS
Copy link

@pi0 you meant no effect on prod bundle?

@TheAlexLichter
Copy link
Member

@AndrewBogdanovTSS No change for the prod bundle, no "noticable" effect for the dev bundle besides IE11 compat ☺️

@pi0 pi0 mentioned this pull request Sep 30, 2019
@AndrewBogdanovTSS
Copy link

AndrewBogdanovTSS commented Oct 2, 2019

Just updated my project to Nuxt v.2.10.0 - IE still throw errors about Consola in dev mode:

SCRIPT1002: Syntax Error
vendors.app.js (36605,1)
    class Consola {...

@clarkdo
Copy link
Member Author

clarkdo commented Oct 2, 2019

I’ll have a look

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.

None yet

7 participants