container.has() behavior has been changed without deprecations warning

[olivierlesnicki]

container.has('route:http://www.google.com') used to return false (pre-Ember 1.13.x), it now throws an error. No deprecation warnings were previously logged.

[stefanpenner]

cc @dgeb

[dgeb]

Looks like that should passing the validateFullName check - see https://github.com/emberjs/ember.js/blob/master/packages/container/lib/registry.js#L655-L660

/^[^:]+.+:[^:]+$/.test('route:http://www.google.com') // => true

has also calls normalize which in turn calls normalizeFullName, which is set to registry.resolver.normalize when the registry is built (see https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L902).

So, this looks to be a breaking change with the resolver (probably from ember-cli) and not the container or registry.

But then again, I think it might be more appropriate for validateFullName to throw an error in this situation. I'd be surprised if that could be a valid route name, but I'm not sure exactly which characters should be restricted.

@stefanpenner thoughts?

[stefanpenner]

it now throws an error.

Can you:

• tell us what version of ember
• provide the error / stack trace
• provide a reproduction – some isolated repo or bin would be ideal

[rwjblue]

container is definitely private API. I would kinda expect container.has('route:http://www.google.com') to throw as that is a wacky route name...

[dgeb]

That's my feeling as well. I don't think this should be getting past validateFullName, but it does currently.

The trick is to ensure that VALID_FULL_NAME_REGEXP accounts for "wacky" so that the right error can be thrown :)

[stefanpenner]

I would love to see the issue in a reproduction, to make sure nothing else funky is going on.

[olivierlesnicki]

This was a tricky one to reproduce: https://github.com/olivierlesnicki/emberjs-issue-12082
It seems the error is only thrown while running tests.

Please see tests/integration/components/bg-breadcrumb, this test was originally written for ember.js 1.11 before the "integration tests" landed.

[olivierlesnicki]

@stefanpenner got a chance to look at the reproduction?

[stefanpenner]

Sorry didn't notice it. Will look once I get to work

[stefanpenner]

"Tried to normalize a container name without a colon (:) in it. You probably tried to lookup a name that did not contain a type, a colon, and a name. A proper lookup name would be `view:post`."
fullName
"route:https:/google.com"

@dgeb we have an assertion for fullName.split(':').length === 2

Im not sure, I think this is fine. We have a trade-off, we can provide good errors when people provide something clearly invalid or we can opt to not do this.

That being said, one option is to make has not assert on invalid names at all. This likely satisfies all our constraints

[olivierlesnicki]

I'm not against it throwing an error. I think my point was more around the fact that a breaking "change" was introduced without the proper preceding deprecation warning...

I've refactored my usage of the .has function to catch any errors.

[rwjblue]

The container is and has always been a private API, we have generally been very clear about that. Use it at your own risk...

We definitely try to deprecate as much intentional change in behavior as possible. In this case we added a helpful error message when the method is used with incorrect arguments, but the fact that it previously worked was not intentional (though perhaps we should make has always return true or false?).

Also, as an aside @dgeb has been doing wonderful work lately to expose the parts that folks need from the container/registry as public API.

[dgeb]

Hey thanks @rwjblue :)

I still think we should be catching this at validateFullName. No good can come from letting it propagate downstream (although I'm glad an assertion is in place there to catch it).

Let's try to beef up the regex, which is currently:

var VALID_FULL_NAME_REGEXP = /^[^:]+.+:[^:]+$/;

[stefanpenner]

(though perhaps we should make has always return true or false

The more i think about it, the more I like it.

Let's try to beef up the regex, which is currently:

👍