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.

Sign up for GitHub

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

Jump to bottom

fix(browser_adapter.ts): baseElement.getAttribute #3214

Closed
wants to merge 1 commit into from
Closed

fix(browser_adapter.ts): baseElement.getAttribute #3214

wants to merge 1 commit into from

Conversation

PatrickJS
Copy link
Member

currently throwing an error

Error during instantiation of LocationStrategy! (RouterLink -> Router -> Location -> LocationStrategy). 
ORIGINAL ERROR: TypeError: baseElement.attr is not a function

closes #3441

tbosch
tbosch reviewed Jul 24, 2015
View reviewed changes
modules/angular2/src/dom/browser_adapter.ts
@@ -287,7 +287,7 @@ function getBaseElementHref(): string {
return null;
}
}
return baseElement.attr('href');
return baseElement.getAttribute('href');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, could you add a unit test for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added tests but ran into a problem with baseElement cache

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a resetBaseElement method to the DomAdapter (and all implementations). You can narrow it down to only the JS/Dart adapter via if (DOM.supportsDOMEvents()) {...}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I did something wrong here when adding resetBaseElement and getBaseHref to the other adapters

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 24, 2015
PatrickJS
PatrickJS reviewed Jul 25, 2015
View reviewed changes
modules/angular2/test/dom/dom_adapter_spec.ts
var $head = DOM.defaultDoc().head;
DOM.appendChild($head, $base);

var subject = DOM.getBaseHref();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fails due to baseElement being cache from the prevous test

@PatrickJS PatrickJS force-pushed the patch-1 branch 2 times, most recently from a9cb434 to b44620a Compare July 28, 2015 00:22
@PatrickJS PatrickJS mentioned this pull request Aug 1, 2015
Closed
@PatrickJS
Copy link
Member Author

I'm having trouble correctly testing this in Dart. Can anyone help?

@btford
Copy link
Contributor

btford commented Aug 3, 2015

@gdi2290 sure– what's the problem?

@btford btford self-assigned this Aug 3, 2015
@btford btford added this to the alpha-34 milestone Aug 3, 2015
@PatrickJS PatrickJS force-pushed the patch-1 branch 2 times, most recently from f32a1ad to e4379ec Compare August 3, 2015 21:04
@PatrickJS
Copy link
Member Author

@btford with Dart I'm trying not run these tests for Dart (since it's only in js atm). Using if (DOM.supportsDOMEvents) travis dart error doesn't seem to work when avoiding the Dart version. I recall an environment variable (IS_DARTIUM) that is used to narrow down a test to js so should I use that rather than supportsDOMEvents as Tobias suggested? Unless he meant for me to write out getBaseHref for Dart too (I know he is on vaca this week)

@btford
Copy link
Contributor

btford commented Aug 4, 2015

Unless he meant for me to write out getBaseHref for Dart too (I know he is on vaca this week)

I think that's what he meant, but I'm not sure. :/

@btford btford modified the milestones: alpha-35, alpha-34 Aug 5, 2015
@btford
Copy link
Contributor

btford commented Aug 5, 2015

Moving this to alpha-35.

@PatrickJS
fix(browser_adapter.ts): baseElement.getAttribute
e8c3e34 
currently throwing an error
```error
Error during instantiation of LocationStrategy! (RouterLink -> Router
-> Location -> LocationStrategy).
ORIGINAL ERROR: TypeError: baseElement.attr is not a function
```
@PatrickJS
Copy link
Member Author

@btford can you take over this PR? you can either close and recreate it or push to my repo git push patrickjs yourbranch:patch-1 for it to show up in this PR. All that's left is Dart support and I don't want this PR to live through another version :/

@btford
Copy link
Contributor

btford commented Aug 18, 2015

Yes, I'll get this in today.

@btford
Copy link
Contributor

btford commented Aug 18, 2015

Fixed and pushed to presubmit. 👍

@btford btford closed this in 235dec2 Aug 18, 2015
@PatrickJS PatrickJS deleted the patch-1 branch August 18, 2015 19:18
@PatrickJS
Copy link
Member Author

thanks! 👍

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@btford btford

Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes effort2: days type: bug/fix
Projects
None yet
Milestone
alpha-35
Development

Successfully merging this pull request may close these issues.

HTML5LocationStrategy doesn't work (even with base attr)
4 participants
@PatrickJS @btford @tbosch @googlebot