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

feature: add static.prefix #2755

Merged
merged 2 commits into from Feb 5, 2018

Conversation

Projects
None yet
4 participants
@clarkdo
Copy link
Member

clarkdo commented Feb 3, 2018

Resolve #2753

Add render.static.prefix to check if need to append router.base to serveStatic

@clarkdo clarkdo requested review from Atinux and pi0 Feb 3, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 3, 2018

Codecov Report

Merging #2755 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #2755      +/-   ##
==========================================
+ Coverage   99.91%   99.91%   +<.01%     
==========================================
  Files          23       23              
  Lines        1211     1213       +2     
==========================================
+ Hits         1210     1212       +2     
  Misses          1        1
Impacted Files Coverage Δ
lib/common/options.js 100% <ø> (ø) ⬆️
lib/core/renderer.js 100% <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 2158327...e60a8e7. Read the comment docs.

@Atinux

This comment has been minimized.

Copy link
Member

Atinux commented Feb 4, 2018

Dont we need a default value?

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Feb 4, 2018

@Atinux

useMiddleware only accepts prefix !== false https://github.com/nuxt/nuxt.js/blob/dev/lib/core/renderer.js#L193, so unless manually config static.prefix: false, it will have the prefix anyway.

)
staticMiddleware.prefix = this.options.render.static.prefix

This comment has been minimized.

@pi0

pi0 Feb 4, 2018

Member

Code readability suggestion: Boolean(this.options.render.static.prefix).

This comment has been minimized.

@clarkdo

clarkdo Feb 4, 2018

Member

It may make unexpected behavior, if not config static.prefix, it will be false and ignore the prefix, but prefix should not be ignored by default.

This comment has been minimized.

@pi0

pi0 Feb 4, 2018

Member

So what if we add a default value true in

static: {},
? This prefix is little confusing now. Because it doesn't means use this prefix string but means just use prefix 😆

This comment has been minimized.

@clarkdo

clarkdo Feb 5, 2018

Member

@pi0 Added the default value.

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Feb 4, 2018

@clarkdo clarkdo force-pushed the clarkdo:static_prefix branch to e60a8e7 Feb 5, 2018

@Atinux Atinux merged commit 0a52cd2 into nuxt:dev Feb 5, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new issues
Details
@lock

This comment has been minimized.

Copy link

lock bot commented Nov 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.