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

cleanup app-link for tests #4526

Merged
merged 9 commits into from
Jul 5, 2018
Merged

cleanup app-link for tests #4526

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
21caa76
cleanup app-link for tests
OmgImAlexis Jul 2, 2018
2886218
remove isAnonymised
OmgImAlexis Jul 3, 2018
ab8e534
moved template property to top
OmgImAlexis Jul 3, 2018
ac325f1
remove redundant component name
OmgImAlexis Jul 3, 2018
d806089
remove MEDUSA from component
OmgImAlexis Jul 3, 2018
e313e43
add review code
OmgImAlexis Jul 4, 2018
76d140a
Fix component registration
sharkykh Jul 4, 2018
2de7d08
`indexerName` computed prop was missing some code
sharkykh Jul 4, 2018
e13d1af
Fix null computedHref when href is loaded after creation
sharkykh Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 90 additions & 60 deletions themes-default/slim/views/vue-components/app-link.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<script>
Vue.component('app-link', {
name: 'app-link',
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is redundant, please remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for future usage.

Copy link
Contributor

@sharkykh sharkykh Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, then remove the one above it, which overrides the first argument to Vue.component.

store,
props: {
to: [String, Object],
href: String,
Expand All @@ -22,80 +23,109 @@ Vue.component('app-link', {
placeholder: {
type: String,
default: 'indexer-to-name'
}
},
base: String
},
computed: {
config() {
return this.$store.state.config;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that for future usage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for future usage.

computedBase() {
// Return prop before HTML element to allow tests to mock
if (this.base) {
return this.base;
}

return document.getElementsByTagName('base')[0].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.

I don't see any problem with doing this:

const { base } = this;
// Return prop before HTML element to allow tests to mock
return this.base || document.getElementsByTagName('base')[0].getAttribute('href');

It's just a personal preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to make sure document isn't ever called since it'll throw an error in the tests. I know how bad that sounds but I'll fix it once I move this to a .vue file.

Copy link
Contributor

@sharkykh sharkykh Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, JavaScript logical statements are evaluated from left to right and not all at once.
Meaning if Boolean(this.base) is true, it will return that value. As long as you pass the base in the tests, should be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not that. It's the testing framework seeing it in the AST that's the issue.

},
computedHref() {
const { indexerId, placeholder } = this;
if (indexerId && placeholder) {
return this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well destructure href from this (note the return statement too)
Also to make the switch later easier, I suggest destructuring config from MEDUSA (one level would suffice)

const { config } = MEDUSA;

}
return this.href;
},
isIRC() {
return this.computedHref.startsWith('irc://');
},
isAbsolute() {
const href = this.computedHref;
return /^[a-z][a-z0-9+.-]*:/.test(href);
},
isExternal() {
const base = this.computedBase;
const href = this.computedHref;
return !href.startsWith(base) && !href.startsWith('webcal://');
},
isHashPath() {
return this.computedHref.startsWith('#');
},
isAnonymised() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It can already be removed. No references remain (on href attributes in mako files).
I'll remove the last one which is the Donations link in another PR (I also changed it on #4519)
It didn't raise an error because it uses this method:

<app-link href="${app.DONATIONS_URL}" rel="noreferrer" onclick="window.open('${app.ANON_REDIRECT}' + this.href); return false;">

const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect && href.startsWith(anonRedirect);
},
anonymisedHref() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect ? anonRedirect + href : href;
},
linkProperties() {
const {to, indexerId, placeholder} = this;
const href = indexerId && placeholder ? this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId)) : this.href;
const base = document.getElementsByTagName('base')[0].getAttribute('href');
const isIRC = url => {
return url.startsWith('irc://');
};
const isAbsolute = url => {
return /^[a-z][a-z0-9+.-]*:/.test(url);
const { to, isIRC, isAbsolute, isExternal, isHashPath, isAnonymised, anonymisedHref } = this;
const base = this.computedBase;
const href = this.computedHref;

// Return normal router-link
if (to) {
return {
is: 'router-link',
to: { name: href }
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note...
Have you tried it? This technique doesn't seem to actually work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How did you test this? The to needs to be the name of a loaded route.

Copy link
Contributor

Choose a reason for hiding this comment

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

When on the help and info page:

<app-link to="not-found">Not Found</app-link>
<app-link to="config">Help &amp; Info</app-link>

And the elements I got in the DOM are:

<a class="router-link-exact-active router-link-active">Not Found</a>
<a class="router-link-exact-active router-link-active">Help &amp; Info</a>

It doesn't work even when I put the links inside the routes, like in config.vue and 404.vue.

Copy link
Contributor

@sharkykh sharkykh Jul 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

<router-link> in the same locations, works just fine.

<router-link :to="{ name: 'not-found' }">Not Found</router-link>
<router-link :to="{ name: 'config' }">Help &amp; Info</router-link>
<a href="/not-found" class="">Not Found</a>
<a href="/config" class="router-link-exact-active router-link-active">Help &amp; Info</a>

Copy link
Collaborator Author

@OmgImAlexis OmgImAlexis Jul 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should replace to: { name: href } with to then it'll just pass the whole object through or we could just straight out remove that ability and move those links to router-link?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should replace to: { name: href } with to then it'll just pass the whole object through

Just tried that, doesn't seem to work still.

or we could just straight out remove that ability and move those links to router-link?

If we can't figure it out, then yes...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you check what the AppLink's computed props were? This is why we need tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall we merge and then fix this in the test PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we merge and then fix this in the test PR?

No, the app link component is broken on this branch.
Give me a few minutes I'll push a commit to fix everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll try and fix this another time. Fixed the other errors.

};
}
const isExternal = url => {
return !url.startsWith(base) && !url.startsWith('webcal://');
};
const isHashPath = url => {
return url.startsWith('#')
};
const isAnonymised = url => {
return MEDUSA.config.anonRedirect && url.startsWith(MEDUSA.config.anonRedirect);
};
const anonymise = url => MEDUSA.config.anonRedirect ? MEDUSA.config.anonRedirect + url : url;
if (!to) {
if (href) {
// @TODO: Remove this once we move to vue only
if (isAnonymised(href)) {
throw new Error('Still using anon_url in Python!');
}
const resolvedHref = () => {
if (isHashPath(href)) {
const {location} = window;
if (location.hash.length === 0) {
// current location might be `url#`
const newHash = location.href.endsWith('#') ? href.substr(1) : href;
return location.href + newHash;
}
return location.href.replace(location.hash, '') + href;
}
if (isIRC(href)) {
return href;
}
if (isAbsolute(href)) {
if (isExternal(href)) {
return anonymise(href)
} else {
return href;
}
} else {
return new URL(href, base).href
}
};
return {
is: 'a',
target: isAbsolute(href) && isExternal(href) ? '_blank' : '_self',
href: resolvedHref(),
rel: isAbsolute(href) && isExternal(href) ? 'noreferrer' : undefined
};
}

// Just return a boring link with other attrs
// @NOTE: This is for scroll achors as it uses the id
// Just return a boring link with other attrs
// @NOTE: This is for scroll achors as it uses the id
if (!href) {
return {
is: 'a',
falseLink: true
};
}

// @TODO: Remove this once we move to vue only
if (isAnonymised) {
throw new Error('Still using anon_url in Python!');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, can be removed.


return {
is: 'router-link',
to: { name: href }
is: 'a',
target: isAbsolute && isExternal ? '_blank' : '_self',
href: (() => {
if (isHashPath) {
const { location } = window;
if (location.hash.length === 0) {
// Current location might be `url#`
const newHash = location.href.endsWith('#') ? href.substr(1) : href;
return location.href + newHash;
}
return location.href.replace(location.hash, '') + href;
}
if (isIRC) {
return href;
}
if (isAbsolute) {
if (isExternal) {
return anonymisedHref;
}
return href;
}
return new URL(href, base).href;
})(),
rel: isAbsolute && isExternal ? 'noreferrer' : undefined
};
}
},
template: `#app-link-template`
template: '#app-link-template'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think our style is to put the template property right below the component name. Let's do that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to create a styles guide for things like this on the wiki.

});
</script>
<style>
Expand Down
150 changes: 90 additions & 60 deletions themes/dark/templates/vue-components/app-link.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<script>
Vue.component('app-link', {
name: 'app-link',
store,
props: {
to: [String, Object],
href: String,
Expand All @@ -22,80 +23,109 @@ Vue.component('app-link', {
placeholder: {
type: String,
default: 'indexer-to-name'
}
},
base: String
},
computed: {
config() {
return this.$store.state.config;
},
computedBase() {
// Return prop before HTML element to allow tests to mock
if (this.base) {
return this.base;
}

return document.getElementsByTagName('base')[0].getAttribute('href');
},
computedHref() {
const { indexerId, placeholder } = this;
if (indexerId && placeholder) {
return this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId));
}
return this.href;
},
isIRC() {
return this.computedHref.startsWith('irc://');
},
isAbsolute() {
const href = this.computedHref;
return /^[a-z][a-z0-9+.-]*:/.test(href);
},
isExternal() {
const base = this.computedBase;
const href = this.computedHref;
return !href.startsWith(base) && !href.startsWith('webcal://');
},
isHashPath() {
return this.computedHref.startsWith('#');
},
isAnonymised() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect && href.startsWith(anonRedirect);
},
anonymisedHref() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect ? anonRedirect + href : href;
},
linkProperties() {
const {to, indexerId, placeholder} = this;
const href = indexerId && placeholder ? this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId)) : this.href;
const base = document.getElementsByTagName('base')[0].getAttribute('href');
const isIRC = url => {
return url.startsWith('irc://');
};
const isAbsolute = url => {
return /^[a-z][a-z0-9+.-]*:/.test(url);
const { to, isIRC, isAbsolute, isExternal, isHashPath, isAnonymised, anonymisedHref } = this;
const base = this.computedBase;
const href = this.computedHref;

// Return normal router-link
if (to) {
return {
is: 'router-link',
to: { name: href }
};
}
const isExternal = url => {
return !url.startsWith(base) && !url.startsWith('webcal://');
};
const isHashPath = url => {
return url.startsWith('#')
};
const isAnonymised = url => {
return MEDUSA.config.anonRedirect && url.startsWith(MEDUSA.config.anonRedirect);
};
const anonymise = url => MEDUSA.config.anonRedirect ? MEDUSA.config.anonRedirect + url : url;
if (!to) {
if (href) {
// @TODO: Remove this once we move to vue only
if (isAnonymised(href)) {
throw new Error('Still using anon_url in Python!');
}
const resolvedHref = () => {
if (isHashPath(href)) {
const {location} = window;
if (location.hash.length === 0) {
// current location might be `url#`
const newHash = location.href.endsWith('#') ? href.substr(1) : href;
return location.href + newHash;
}
return location.href.replace(location.hash, '') + href;
}
if (isIRC(href)) {
return href;
}
if (isAbsolute(href)) {
if (isExternal(href)) {
return anonymise(href)
} else {
return href;
}
} else {
return new URL(href, base).href
}
};
return {
is: 'a',
target: isAbsolute(href) && isExternal(href) ? '_blank' : '_self',
href: resolvedHref(),
rel: isAbsolute(href) && isExternal(href) ? 'noreferrer' : undefined
};
}

// Just return a boring link with other attrs
// @NOTE: This is for scroll achors as it uses the id
// Just return a boring link with other attrs
// @NOTE: This is for scroll achors as it uses the id
if (!href) {
return {
is: 'a',
falseLink: true
};
}

// @TODO: Remove this once we move to vue only
if (isAnonymised) {
throw new Error('Still using anon_url in Python!');
}

return {
is: 'router-link',
to: { name: href }
is: 'a',
target: isAbsolute && isExternal ? '_blank' : '_self',
href: (() => {
if (isHashPath) {
const { location } = window;
if (location.hash.length === 0) {
// Current location might be `url#`
const newHash = location.href.endsWith('#') ? href.substr(1) : href;
return location.href + newHash;
}
return location.href.replace(location.hash, '') + href;
}
if (isIRC) {
return href;
}
if (isAbsolute) {
if (isExternal) {
return anonymisedHref;
}
return href;
}
return new URL(href, base).href;
})(),
rel: isAbsolute && isExternal ? 'noreferrer' : undefined
};
}
},
template: `#app-link-template`
template: '#app-link-template'
});
</script>
<style>
Expand Down
Loading