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

Fix resolving @scope/name namespaces #531

Merged
merged 2 commits into from
Apr 11, 2020

Conversation

buschtoens
Copy link
Contributor

It is possible resolve modules from other packages using namespaces. For instance, the following looks up the intl service directly from the ember-intl package:

this.owner.lookup('ember-intl@service:intl');
this.owner.lookup('service:ember-intl@intl');

Notably, this instance is !== to the app re-export that is merged with the host app:

this.owner.lookup('service:intl');

This is expected behavior.

So generalizing, the pattern for looking up something from a foreign package is:

this.owner.lookup(`${packageName}@{type}:${name}`);
this.owner.lookup(`{type}:${packageName}@${name}`);

Unfortunately, this does not work for packages that have a scoped name, e.g. @corp/fancy-addon.

The invocation would look like:

this.owner.lookup('@corp/fancy-addon@service:intl');
this.owner.lookup('service:@corp/fancy-addon@intl');

parseName(fullNane) does not support this, as it was written before package scopes were introduced and only expected a single @ to be present.

function parseName(fullName) {
if (fullName.parsedName === true) { return fullName; }
let prefix, type, name;
let fullNameParts = fullName.split('@');
if (fullNameParts.length === 2) {
let prefixParts = fullNameParts[0].split(':');
if (prefixParts.length === 2) {
if (prefixParts[1].length === 0) {
type = prefixParts[0];
name = `@${fullNameParts[1]}`;
} else {
prefix = prefixParts[1];
type = prefixParts[0];
name = fullNameParts[1];
}
} else {
let nameParts = fullNameParts[1].split(':');
prefix = fullNameParts[0];
type = nameParts[0];
name = nameParts[1];
}
if (type === 'template' && prefix.lastIndexOf('components/', 0) === 0) {
name = `components/${name}`;
prefix = prefix.slice(11);
}
} else {
fullNameParts = fullName.split(':');
type = fullNameParts[0];
name = fullNameParts[1];
}
let fullNameWithoutType = name;
let namespace = get(this, 'namespace');
let root = namespace;
return {
parsedName: true,
fullName: fullName,
prefix: prefix || this.prefix({type: type}),
type: type,
fullNameWithoutType: fullNameWithoutType,
name: name,
root: root,
resolveMethodName: "resolve" + classify(type)
};
}

This PR, so far, adds failing tests for scoped package names.

Is this something that we would want to fix? I personally have an interest in it and would finish the PR, if so.

Related

@buschtoens buschtoens marked this pull request as ready for review March 30, 2020 16:12
@buschtoens
Copy link
Contributor Author

I seem to have fixed it. 🤔

All existing tests are still passing and the newly added tests pass as well.

@buschtoens
Copy link
Contributor Author

buschtoens commented Mar 30, 2020

CI is failing for an unrelated reason:

error [email protected]: The engine "node" is incompatible with this module. Expected version ">=10". Got "8.17.0"

error Found incompatible module.

info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

The command "yarn install --no-lockfile --non-interactive" failed and exited with 1 during .

Fixing in #532.

@buschtoens
Copy link
Contributor Author

Assuming CI is fixed (#532), would this PR be mergeable? Is there something I need to improve or do we not want to support this in the first place?

@buschtoens
Copy link
Contributor Author

The PR is rebased and CI is passing. ✅

@rwjblue rwjblue added the bug label Apr 11, 2020
@rwjblue rwjblue merged commit f142698 into ember-cli:master Apr 11, 2020
@rwjblue
Copy link
Member

rwjblue commented Apr 11, 2020

Thank you @buschtoens!

@buschtoens
Copy link
Contributor Author

Awesome! Thank you for merging this! 💖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants