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: generate failure #5007

Merged
merged 18 commits into from
Feb 13, 2019
Merged

fix: generate failure #5007

merged 18 commits into from
Feb 13, 2019

Conversation

karakum
Copy link

@karakum karakum commented Feb 11, 2019

Allow to use [name].js?v=[contenthash] in chunk names.

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

In continuos of #3387 #4073
Resolves: #5006

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.

@karakum karakum changed the title fix: generate failure (#5006) fix: generate failure Feb 11, 2019
@codecov-io
Copy link

codecov-io commented Feb 11, 2019

Codecov Report

Merging #5007 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5007      +/-   ##
==========================================
+ Coverage   94.91%   94.92%   +0.01%     
==========================================
  Files          72       72              
  Lines        2400     2405       +5     
  Branches      607      608       +1     
==========================================
+ Hits         2278     2283       +5     
  Misses        102      102              
  Partials       20       20
Impacted Files Coverage Δ
packages/webpack/src/plugins/vue/server.js 86.2% <100%> (+1.59%) ⬆️
packages/webpack/src/plugins/vue/util.js 72.72% <100%> (+6.06%) ⬆️

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 21e7936...af0c596. Read the comment docs.

@manniL manniL requested review from pi0 and clarkdo and removed request for pi0 February 11, 2019 20:52
pi0
pi0 previously approved these changes Feb 11, 2019
@karakum
Copy link
Author

karakum commented Feb 12, 2019

@manniL Who or what we are waiting for?

@manniL
Copy link
Member

manniL commented Feb 12, 2019

@karakum The PR hasn't even been on for 24 hours. Please be a little patient so that other team members have a chance to review the code as well ☺️

@manniL
Copy link
Member

manniL commented Feb 12, 2019

@karakum However, a fixture to test the correct behavior (and to therefore avoid regressions) would be highly appreciated 🙌

@Atinux
Copy link
Member

Atinux commented Feb 12, 2019

I was writing my comment to ask you to add a fixture and a unit test...
Well you just made it before I clicked "comment" 👏

I'm good with the PR if the test are all green

@Atinux
Copy link
Member

Atinux commented Feb 12, 2019

ESLint is broken: https://circleci.com/gh/nuxt/nuxt.js/29158?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Should we add a precommit hook for it @manniL @pi0 ?

@manniL
Copy link
Member

manniL commented Feb 12, 2019

@Atinux Yup, that's a great idea!

@karakum
Copy link
Author

karakum commented Feb 12, 2019

@Atinux All checks have passed! 😎

@clarkdo
Copy link
Member

clarkdo commented Feb 12, 2019

Could we move test to a existed fixture since a new fixture may increase more CI time consuming?

@karakum
Copy link
Author

karakum commented Feb 12, 2019

I think no.
Main settings

  build: {
    filenames: {
      app: '[name].js?v=[contenthash]',
      chunk: '[name].js?v=[contenthash]'
    }
  }

can't be turned on/off while unit-test, only just in whole fixture. Am I wrong?

@clarkdo clarkdo merged commit bcd672f into nuxt:dev Feb 13, 2019
@manniL
Copy link
Member

manniL commented Feb 13, 2019

@karakum Many thanks for your contribution 🙏

@galvez
Copy link

galvez commented Feb 18, 2019

I was bit by this one -- many thanks @karakum :)

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.

Cannot find module 'pages/index.js?v=d187aabeaedb9b4063f6' from '/sandbox'
8 participants