Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($compile): show module name during multidir error #11829

Closed

Conversation

lugovsky
Copy link
Contributor

@lugovsky lugovsky commented May 7, 2015

Show module name if possible when multidir error happens.

Closes #11775

P.S. Hey guys, please check out this solution. Let me know if the way I implemented it looks correct at least from afar.
Honestly, I've spent a lot of time trying to understand the way, that affects core functionality as less as possible and I think that's it. But if you see some another solution, I will be glad to hear that.
In case you think that these changes are critical and this feature doesn't worth it, I'm ready to this as well :).

@kentcdodds
Copy link
Member

Looks good to me :-)

@lgalfaso
Copy link
Contributor

I see this as a real issue, but there are a few things that needs some cleanup.

  • It exposes angular specific properties with a _ prefix and not with $$
  • Exposing the factory looks like a bad idea, why not expose the module name itself?
  • A test when the module name is missing, is missing :)
  • (nice to have) A test with a directive that has an interceptor

All this a side, I think this solves a real struggle

@lugovsky
Copy link
Contributor Author

Sorry guys for the delay in response. Unfortunately I've caught some illness which didn't let me do anything.
@lgalfaso I changed code to achieve your points 1 and 3. I didn't actually get about what interceptor you told in point 4.
There is one problem implementing point 2. Unfortunately directive factory doesn't have any back reference to module in which it was defined. As you can see, in my original solution, to understand in which module the directive was defined I'm doing a search through invokeQueue's of all loaded modules. The idea was, that the code was executing only when failure was detected, so it shouldn't affect performance. But if I will use that search every time directive object is creating, that would definitely affect performance. So that's what I don't want to do :). And I don't see any good solution here.
One the other hand, there is some other solution. In that solution small modifications to loader should be done: special case of invokeLater should be created for directive. I've made some sample here akveoDev@6eb899e . That as well will put most of code I created in this PR to the trash bin, but as well will reduce the complexity of this task.

@lgalfaso
Copy link
Contributor

@lugovsky I like the path that akveoDev@6eb899e is going. In fact if this would be added strait to invokeLater (with the exception of config, constant and value). I also think that this would make this PR a lot cleaner

@lugovsky
Copy link
Contributor Author

@lgalfaso Did I got you correctly and you wish to add that code straight to invokeLater, so every factory/service/provider/decorator/animation/filter/controller (except config/constant/value) has string representing module in which it was defined as well? Do you want to provide additional extensibility for those recipies in the future?

@lgalfaso
Copy link
Contributor

@lugovsky yes, and yes. The later is to show in the error the module when a dependency is not fulfill.

Just to be sure, I would like to keep the current scope of this PR.

In the future (PR welcome) showing the module when a dependency is not fulfill would be nice.

Show module name if possible when multidir error happens.

Closes angular#11775
@lugovsky lugovsky force-pushed the multidir-descriptive-message branch from b4de9ed to b370b7e Compare May 19, 2015 20:15
@lugovsky
Copy link
Contributor Author

@lgalfaso Made updates according to our discussion. I like it much more this way actually :). Please make another review and suggest new changes if any.

If you want me to handle showing the module when a dependency is not fulfilled, please create or find the issue with the appropriate description :). Honestly, I haven't got the idea. But anyway I will be glad to help you with this problem!

@lgalfaso lgalfaso assigned lgalfaso and unassigned petebacondarwin May 20, 2015
@lgalfaso
Copy link
Contributor

LGTM, will merge after the cut

@lugovsky
Copy link
Contributor Author

@lgalfaso Awesome, thanks!

PS: If you need help with that problem when dependency is not fulfilled, just create the issue after the merge and reference me. I will be happy to help :)

@lgalfaso
Copy link
Contributor

lgalfaso commented Jun 1, 2015

landed as 351fe4b

@lgalfaso lgalfaso closed this Jun 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message on directive namespace conflict
5 participants