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

feat(load): resolve plugins correctly if provided as relative or absolute path #2401

Closed

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Jan 11, 2021

Description

fixes #932

Motivation and Context

Usage examples

How Has This Been Tested?

Unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

let normalizedName = name;

if (
path.isAbsolute(name) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://nodejs.org/api/path.html#path_path_isabsolute_path

For example, on POSIX:

path.isAbsolute('/foo/bar'); // true
path.isAbsolute('/baz/..');  // true
path.isAbsolute('qux/');     // false
path.isAbsolute('.');        // false

On Windows:

path.isAbsolute('//server');    // true
path.isAbsolute('\\\\server');  // true
path.isAbsolute('C:/foo/..');   // true
path.isAbsolute('C:\\foo\\..'); // true
path.isAbsolute('bar\\baz');    // false
path.isAbsolute('bar/baz');     // false
path.isAbsolute('.');           // false

Comment on lines +27 to +30
name.startsWith('./') ||
name.startsWith('../') ||
name.startsWith('.\\') ||
name.startsWith('..\\')
Copy link
Contributor Author

@armano2 armano2 Jan 11, 2021

Choose a reason for hiding this comment

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

this could be done in form of regexp

/^\.{1,2}(\/|\\)/g

https://regexr.com/5k20p

Copy link
Member

Choose a reason for hiding this comment

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

True, personally I like the readability of the current code as long as won't grow to more cases.

@armano2 armano2 marked this pull request as draft January 12, 2021 08:41
@armano2 armano2 force-pushed the fix/local-plugins branch 4 times, most recently from 5d6dc4f to 0803cf6 Compare January 12, 2021 12:42
@armano2 armano2 marked this pull request as ready for review January 12, 2021 13:58
@escapedcat escapedcat requested a review from byCedric January 13, 2021 05:59
@armano2
Copy link
Contributor Author

armano2 commented Jan 16, 2021

I'm no longer sure about this change, user should just require('./some/path') to load plugin as local, and this is already supported

@escapedcat
Copy link
Member

Hm, as you mentioned this works:

{
  plugins: [require('./dir/my-custom-plugin.js')],
}

I think it's fine to point out in the docs to use this and not paths directly in the array. Seems cleaner to me.

@armano2
Copy link
Contributor Author

armano2 commented Jan 17, 2021

ok, than i'm going to close this PR, as those changes seem less stable compared to providing functions dirrectly

@armano2 armano2 closed this Jan 17, 2021
@armano2 armano2 deleted the fix/local-plugins branch January 17, 2021 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

commitlint does not find plugin when a relative path is provided
2 participants