salsify / ember-css-modules

CSS Modules for ambitious applications
MIT License
282 stars 49 forks source link

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

Closed Eric162 closed 2 years ago

Eric162 commented 3 years ago

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 commented 3 years ago

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

dfreeman commented 2 years ago

Thank you, @Eric162!