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(vue-app): multiple named views cause invalid syntax #5262

Merged
merged 6 commits into from
Mar 19, 2019

Conversation

leahciMic
Copy link

Fixes #5096

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

  • Reset resMap to an empty string before building the list of components
  • Add another.vue which fails the build if the bug is present (as the
    resulting .nuxt/router.js will contain a syntax error)

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.

Fixes nuxt#5096

* Reset resMap to an empty string before building the list of components
* Add another.vue which fails the build if the bug is present (as the
resulting .nuxt/router.js will contain a syntax error)
@galvez galvez changed the title Fix multiple named views cause invalid syntax fix(vue-app): multiple named views cause invalid syntax Mar 17, 2019
@robertpiosik
Copy link

robertpiosik commented Mar 17, 2019

Faced this issue till the morning, trying to find out a solution and just came across this PR. Simple solution, working as expected 👏

I can't believe that named views was released in stable with such a bug 👀

As you changed "test/fixtures/named-views/nuxt.config.js" file; used filter method on routes will remove those which not satisfy ones in passed function, what breaks autogenerated router configuration. Am I wrong? Correct me if so

@codecov-io
Copy link

codecov-io commented Mar 18, 2019

Codecov Report

Merging #5262 into dev will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5262      +/-   ##
==========================================
+ Coverage   95.95%   95.99%   +0.04%     
==========================================
  Files          74       74              
  Lines        2499     2499              
  Branches      634      634              
==========================================
+ Hits         2398     2399       +1     
+ Misses         85       84       -1     
  Partials       16       16
Impacted Files Coverage Δ
packages/vue-renderer/src/renderer.js 94.89% <0%> (+0.51%) ⬆️

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 ca1ecf0...18f973c. Read the comment docs.

@leahciMic
Copy link
Author

@robertpiosik

Faced this issue till the morning, trying to find out a solution and just came across this PR. Simple solution, working as expected 👏

Thank you for confirming this fixes the issue!

I can't believe that named views was released in stable with such a bug 👀

These things happen!

As you changed "test/fixtures/named-views/nuxt.config.js" file; used filter method on routes will remove those which not satisfy ones in passed function, what breaks autogenerated router configuration. Am I wrong? Correct me if so

I believe you can return a new array from extendRoutes that will replace the routes, but in this case we only modify some of the objects in the array. filter itself does not mutate the array, so it will not drop any routes.

Copy link
Member

@manniL manniL left a comment

Choose a reason for hiding this comment

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

Thanks so far! Left you some comments ☺️

test/fixtures/named-views/nuxt.config.js Outdated Show resolved Hide resolved
test/fixtures/named-views/nuxt.config.js Outdated Show resolved Hide resolved
manniL and others added 2 commits March 19, 2019 06:37
Co-Authored-By: leahciMic <leahciMic@users.noreply.github.com>
@leahciMic
Copy link
Author

@manniL thank you for your review, I have addressed the feedback

Copy link
Member

@manniL manniL left a comment

Choose a reason for hiding this comment

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

LGTM ☺️

@manniL
Copy link
Member

manniL commented Mar 19, 2019

#5096 (comment) <- 🤔

@clarkdo clarkdo merged commit d03a61b into nuxt:dev Mar 19, 2019
@pi0 pi0 mentioned this pull request Mar 20, 2019
@leahciMic
Copy link
Author

@manniL I have looked at that comment, I believe there was some confusion as to how to test it. He has checked out my dev branch of nuxt which is very much out-of-date. I'm clearing up the instructions for him on how to test it.

@danielroe danielroe added the 2.x label Jan 18, 2023
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.

multiple named views cause router.js contains syntax error
6 participants