Skip to content

Alias resolution #81

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

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

Conversation

OscarBarrett
Copy link

Following on from #17 with added tests.

This PR adds support for two new options:

  • root - A relative path string (Starting from CWD) that is used for alias resolution.
  • alias - An object describing aliases for module resolution where the key is the alias to be used and the value is a string or array of paths.

Example config:

['babel-plugin-inline-react-svg', {
  root: path.resolve(__dirname, 'src'),
  alias: {
    images: 'images',
    icons: 'icons'
  }
}],

Using an array for the value of a particular alias allows for fallbacks to other locations, for example a component library.

['babel-plugin-inline-react-svg', {
  root: path.resolve(__dirname),
  alias: {
    images: ['src/images', 'node_modules/my-component-lib/images']
  }
}],

@OscarBarrett
Copy link
Author

@ljharb any feedback on this?

@nrako
Copy link

nrako commented May 4, 2021

It would be nice to have any statement from a maintainer. This repo seems to be still somehow maintained but if that's not the case or if this is only meant to be for "airbnb" private use-cases I believe the community would be grateful if this was clearly stated.

@ljharb
Copy link
Collaborator

ljharb commented May 4, 2021

@nrako it is still maintained; there hasn’t been time to review this PR yet.

if (caseSensitive && !fileExistsWithCaseSync(svgPath)) {
throw new Error(`File path didn't match case of file on disk: ${svgPath}`);
const aliasPart = importPath.split('/')[0];
const aliasMatch = alias ? alias[aliasPart] : undefined;
Copy link
Collaborator

@ljharb ljharb Feb 4, 2022

Choose a reason for hiding this comment

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

What if someone imports from __proto__ or toString ?


if (aliasMatch) {
const resolveRoot = resolvePath(process.cwd(), root || './');
const aliasMatches = typeof aliasMatch === 'string' ? [aliasMatch] : aliasMatch;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const aliasMatches = typeof aliasMatch === 'string' ? [aliasMatch] : aliasMatch;
const aliasMatches = [].concat(aliasMatch);

}

if (svgPath && existsSync(svgPath)) {
chosenPath = svgPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this could use .find ? (or the array.prototype.find package)

@ljharb ljharb reopened this Oct 18, 2022
@ljharb
Copy link
Collaborator

ljharb commented Oct 18, 2022

@OscarBarrett i'd still love to see this PR landed, if you're interested.

@nrako
Copy link

nrako commented Jan 23, 2023

I'd also be somehow glad to see this PR land but I can't blame the author to have lost interest after 2 years of no followups from the maintainers. Perhaps a maintainer could take over though, not sure if the author had given edit permission on his PR but I'm sure it wouldn't be too hard to pick the changes for a new PR if necessary.

@OscarBarrett
Copy link
Author

Allow edits from maintainers is enabled, feel free to do whatever with this PR 👍
Note this has also been published for 3 years at https://www.npmjs.com/package/@oscarbarrett/babel-plugin-inline-react-svg

@ljharb
Copy link
Collaborator

ljharb commented Jan 23, 2023

@nrako there were two comments from me that the author didn’t respond to, to be clear.

@ljharb
Copy link
Collaborator

ljharb commented Jan 23, 2023

@OscarBarrett thats fine, I’ll repurpose the PR eventually, but it’d be nice to get the changes published in the real package instead of a fork.

@nrako
Copy link

nrako commented Jan 24, 2023

@Jiharb – to be really clear, this PR got left without answer from any maintainer for 355 days. The first somewhat helpful interaction (pr reviews) started about 21 months after the initial submission of this PR. There was also an even more older submission #17 with some other defensive interactions.

It certainly would be nice to have certain changes published under the repo which is generally ahead. Otherwise, the "real package" can always be taken somewhere else.

@ljharb
Copy link
Collaborator

ljharb commented Jan 24, 2023

@nrako yes, that’s true. However, I’d hope that folks would have a bit more tolerance for an unpaid maintainer during a global pandemic taking awhile to respond to a PR. The real package will always be here, but as always in open source, if you want to take on the unpaid burden of maintaining a fork, you go right ahead.

@nrako
Copy link

nrako commented Jan 24, 2023

@ljharb – I didn't meant to appear "intolerant" if that's your perception I'm sorry for that – I was only trying to present or correct certain facts about my perception of the current status quo and its history. I'd also have offered to contribute and help break the status quo but unfortunately I presently have the feeling that my efforts could be vain or left in the blue.

So please ignore me; I won't be intervening anymore. I have simple expectations with the interactions and the tones of discussions when I put efforts somewhere (paid or unpaid). And otherwise, I'm also very Open with "maintaining a fork" – that's the rule of the game and hopefully anyone using OSS should understand it. ;)

all the best

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.

4 participants