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

Working dir resolution for middleware #330

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

Conversation

rasmniel
Copy link

Prepend process.cwd() to to supplied middleware path in order to resolve the working directory from cli execution.

I'm unsure what the point of the bare require(mw) statement was, as I found it hard pressed to match anything useful other than absolute paths. If you believe this is required to maintain backwards compat, let me know and I will rethink the PR.

Prepend process.cwd() to to supplied middleware path in order to resolve the working directory from cli execution.
@1j01
Copy link

1j01 commented Oct 24, 2019

path.resolve(process.cwd(), mw) would support absolute paths easily enough.

This would still be a breaking change tho, for the relative case. To maintain backwards compatibility we could check for both locations with a try-catch, and rethrow in the catch for errors that aren't not found errors.

var m;
try {
    m = require(modulePath);
} catch (e) {
    if (e.code !== 'MODULE_NOT_FOUND') {
        throw e;
    }
    m = require(modulePathLegacy);
}

That might be a little weird, but it could be for a deprecation period, or whatever.

@rasmniel
Copy link
Author

Great idea! I've updated the PR as per your suggestion. It should now be possible to resolve middleware both relatively to working directory and relatively to the live-server installation folder. Absolute resolution is also still an option.

I checked that both clauses work. Feel free to verify that and let me know if you find errors.
As for deprecation; I'll let someone else figure out how and when to remove the backwards compatible path resolution — if it should be removed at all 🤷‍♂

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.

2 participants