Skip to content

fixes resolve-path logic for finding local paths that include an addon's name #250

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

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

Eric162
Copy link

@Eric162 Eric162 commented Sep 23, 2021

Right now, if a component file includes the name of its containing addon, the old logic would find that appOrAddonDirIndex would not be -1, when in reality it should be.

EX: addon name is special-button

/* special-button/addon/components/special-button.css */

.special-button {
  composes: special from 'special-button/components/base-specialness';
}

With parameters like this (this is how it looks in our embroider build):

importPath        === 'special-button/components/base-specialness';
fromFile          === '/tmp/broccoli-4392qIAomevRxZbJ/out-03-module_source_funnel/components/special-button.css';
options.root      === '/tmp/broccoli-4392qIAomevRxZbJ/out-03-module_source_funnel/';
options.ownerName === 'special-button';

In the old logic, we would get something like:

let appOrAddonDirIndex = fromFile.indexOf(options.ownerName, options.root.length)
// where
options.root.length === 59;
// so it's searching this string: 'components/special-button.css' for 'special-button' and the result is:
appOrAddonDirIndex === 70;
// then we get to the line:
let prefix = fromFile.substring(0, appOrAddonDirIndex);
prefix === 'special-button.css'
// and the line
let absolutePath = ensurePosixPath(path.resolve(prefix, importPath));
// is trying to resolve
'special-button.css/special-button/components/base-specialness';
// which obviously won't exist

the new code follows more closely with the intent of what was being done here:

// we want to check that after the root ( plus one char for '/'), the fromFile starts with the ownerName
// const fromFileStartsWithOwner = fromFile.substring(options.root.length + 1).startsWith(options.ownerName);
const fromFileStartsWithOwner = 'components/special-button.css'.startsWith('special-button')
// so
fromFileStartsWithOwner === false
// because the portion of fromFile after the owner.root does not start with ownerName, we need to remove the ownerName (plus a '/') from the importPath
importPath = importPath.substring(options.ownerName.length+1);
importPath === 'components/base-specialness';
// now, rather than needing a prefix, we can always use the root, and the new importPath
// so we can try to resolve:
'/tmp/broccoli-4392qIAomevRxZbJ/out-03-module_source_funnel/components/base-specialness.css';
// which should actually exist if that file exists in the addon.

@rwjblue
Copy link

rwjblue commented Sep 24, 2021

@dfreeman - FWIW, this fixes one of the compatibility issues we identified with Embroider in a fairly large app at work.

@dfreeman
Copy link
Member

Thank you, @Eric162!

@dfreeman dfreeman merged commit dbd3154 into salsify:master Nov 19, 2021
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