Skip to content

[mediator/client]fix issue plugin option can't use relative path.#43

Open
takeshi-sakurai wants to merge 3 commits into
masterfrom
feature/plugin-relative-path
Open

[mediator/client]fix issue plugin option can't use relative path.#43
takeshi-sakurai wants to merge 3 commits into
masterfrom
feature/plugin-relative-path

Conversation

@takeshi-sakurai
Copy link
Copy Markdown
Contributor

sgmed can't accept relative path in plugin option.
I change plugin option relative path convert to absolute path on this pull request.

  • sed command got correctly result with absolute plugin path.
  • sed command got correctly result with relative plugin path.


for (let i = 0; i < args.plugins.length; i++) {
const plugin = args.plugins[i];
if (isRelativePath(plugin)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

path.isAbsolute is more handy.

> path.isAbsolute('/aaaa/bbbb/cccc')
true
> path.isAbsolute('/aaaa/../cccc')
true
> path.isAbsolute('./aaaa/cccc')
false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In this case, Is it make mistakes?

> path.isAbsolute('ModuleName')
false

It isn't relative path.
!absolute !== relative.

Copy link
Copy Markdown
Contributor

@dolow dolow Jun 11, 2019

Choose a reason for hiding this comment

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

So there is another path variant.

  • absolute
  • relative
  • resolved

In this case, this fix may be OK.
I'd like to forget about $NODE_PATH because it can be defined by user or user's environment, will you be supporting for this variable ?

Copy link
Copy Markdown
Contributor Author

@takeshi-sakurai takeshi-sakurai Jun 11, 2019

Choose a reason for hiding this comment

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

It basically worked correctly, as far as I tried.
The code below is worked correctly.

const Plugin = require(plugins[i] as string).default;

But, The following points need to be corrected.
Plugins have been removed by:

const spaceSeparatedPaths = (value: string): string[] => {
const parts = [];
const frags = value.split(' ');
for (let i = 0; i < frags.length; i++) {
const frag = frags[i];
if (fs.existsSync(frag)) {
parts.push(frag);
} else {
const nextFrag = frags[i + 1];
if (!nextFrag) {
break;
}
frags[i + 1] = `${frag} ${nextFrag}`;
}
}
return parts;
};

Copy link
Copy Markdown
Contributor Author

@takeshi-sakurai takeshi-sakurai Jun 12, 2019

Choose a reason for hiding this comment

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

Added support for loading plugins by specifying "ModuleName".
dd1b435 94bfbba

@takeshi-sakurai takeshi-sakurai force-pushed the feature/plugin-relative-path branch from 3dae03e to 94bfbba Compare June 14, 2019 02:17
Copy link
Copy Markdown
Contributor

@dolow dolow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@Egliss Egliss left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants