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

eliminate baseDir and support mounting, close #235 #237

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

panlina
Copy link

@panlina panlina commented Oct 30, 2018

As addressed in #235 , baseDir is eliminated and mounting is supported to achieve the same purpose.

Instead of
app.use(ecstatic({.., baseDir: "a"}));,
we now do:
app.use('/a', ecstatic({..}));.

@codecov-io
Copy link

codecov-io commented Oct 30, 2018

Codecov Report

Merging #237 into master will decrease coverage by 0.46%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   75.41%   74.95%   -0.47%     
==========================================
  Files           9        9              
  Lines         541      539       -2     
  Branches      125      127       +2     
==========================================
- Hits          408      404       -4     
  Misses         46       46              
- Partials       87       89       +2
Impacted Files Coverage Δ
lib/ecstatic/opts.js 77.96% <ø> (ø) ⬆️
lib/ecstatic.js 72.5% <0%> (-0.53%) ⬇️
lib/ecstatic/show-dir/index.js 73.41% <0%> (-1.59%) ⬇️

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 818744f...6962ece. Read the comment docs.

@panlina
Copy link
Author

panlina commented Oct 30, 2018

@jfhbrook The codecov check fails. Does that mean I need add tests? I'm not familiar with coverage tools and how they count. You can review my changes and tell me if there's anything I need to do.

@imcuttle
Copy link

imcuttle commented Nov 1, 2018

@panlina I guess you do not need the complex processing,

Just wrap ecstatic simply by #235 (comment)

@panlina
Copy link
Author

panlina commented Nov 1, 2018

@imcuttle It's not "complex processing". It's simplification. 😄 . I saw your solution. It may work but that's a temporary solution as you said.

@jfhbrook
Copy link
Owner

jfhbrook commented Apr 5, 2019

Forgive my thread necromancy!

My concern with this PR as it stands is that a lot of people use this module outside the context of ecstatic, where this flag is actually pretty useful.

Unfortunately I don't use express very often and certainly don't use this module with express! so I don't actually understand the desired behavior with express that well or why basedir breaks it. Is there a way to get this behavior without removing the baseDir option?

@jfhbrook
Copy link
Owner

jfhbrook commented Apr 5, 2019

Something that might be helpful for me is breaking tests for the behavior in express - this would give me a spec to work against.

@panlina
Copy link
Author

panlina commented Apr 6, 2019

Good to see you back after months!

Actually I changed my mind after that. It's better to keep it not relying on a hosting framework as since it's born.

Thank you for taking care of this PR. Now you can close it.

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

Successfully merging this pull request may close these issues.

None yet

4 participants