cleanup app-link for tests #4526

OmgImAlexis · 2018-07-02T16:12:40Z

This will allow us to use Ava tests against this component once #4374 is merged.

sharkykh · 2018-07-02T16:19:44Z

themes-default/slim/views/vue-components/app-link.mako

+                return this.base;
+            }
+
+            return document.getElementsByTagName('base')[0].getAttribute('href');


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.

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.

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.

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

sharkykh · 2018-07-02T16:22:48Z

themes-default/slim/views/vue-components/app-link.mako

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


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;

sharkykh · 2018-07-02T16:37:57Z

themes-default/slim/views/vue-components/app-link.mako

+        isHashPath() {
+            return this.computedHref.startsWith('#');
+        },
+        isAnonymised() {


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;">

sharkykh · 2018-07-02T16:39:38Z

themes-default/slim/views/vue-components/app-link.mako

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


Same, can be removed.

sharkykh · 2018-07-02T16:41:13Z

themes-default/slim/views/vue-components/app-link.mako

            };
        }
    },
-    template: `#app-link-template`
+    template: '#app-link-template'


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

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

sharkykh · 2018-07-02T16:42:18Z

themes-default/slim/views/vue-components/app-link.mako

@@ -13,6 +13,7 @@
 <script>
 Vue.component('app-link', {
    name: 'app-link',


This line is redundant, please remove it.

This is for future usage.

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

sharkykh · 2018-07-02T16:43:37Z

themes-default/slim/views/vue-components/app-link.mako

    },
    computed: {
+        config() {
+            return this.$store.state.config;
+        },


Is that for future usage?

This is for future usage.

Stale

sharkykh · 2018-07-03T15:07:18Z

themes-default/slim/views/vue-components/app-link.mako

+                if (config.indexers[indexer].id === parseInt(indexerId, 10)) {
+                    return config.indexers[indexer].name;
+                }
+            })[0];


The keys of config.indexers don't contain the indexer names,
it's config.indexers.config.indexers. see below [1].
It's because of the MEDUSA.config structure. When we stop using it we should be able to fix that...

This can use find instead of filter, and be much simpler.
This array iteration is so confusing. We don't return the name, we return the key (which is the identifier).
name=TVDBv2 | identifier=tvdb

const { indexers } = config.indexers.config; // This is weird, I know... // Returns `undefined` if not found. return Object.keys(indexers).find(indexer => indexers[indexer].id === parseInt(indexerId, 10));

[1]

{ store.state "config": { "indexers": { "config": { "indexers": { "tvdb": { "xemOrigin": "tvdb", "apiParams": { "useZip": true, "language": "en" }, "showUrl": "https://www.thetvdb.com/dereferrer/series/", "id": 1, "icon": "thetvdb16.png", "name": "TVDBv2", "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tvdb.json", "enabled": true, "baseUrl": "https://api.thetvdb.com/", "mappedTo": "tvdb_id", "identifier": "tvdb" }, "tmdb": { "xemOrigin": null, "apiParams": { "useZip": true, "language": "en" }, "showUrl": "https://www.themoviedb.org/tv/", "id": 4, "icon": "tmdb16.png", "name": "TMDB", "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tmdb.json", "enabled": true, "baseUrl": "https://www.themoviedb.org/", "mappedTo": "tmdb_id", "identifier": "tmdb" }, "tvmaze": { "xemOrigin": null, "apiParams": { "useZip": true, "language": "en" }, "showUrl": "http://www.tvmaze.com/shows/", "id": 3, "icon": "tvmaze16.png", "name": "TVmaze", "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tvmaze.json", "enabled": true, "baseUrl": "http://api.tvmaze.com/", "mappedTo": "tvmaze_id", "identifier": "tvmaze" } }, "main": { "externalMappings": {<truncated>}, "statusMap": {<truncated>}, "traktIndexers": {<truncated>}, "validLanguages": [<truncated>], "langabbvToId": {<truncated>} } } } } }

sharkykh · 2018-07-04T14:38:05Z

themes-default/slim/views/vue-components/app-link.mako

+            if (to) {
+                return {
+                    is: 'router-link',
+                    to: { name: href }


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

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

When on the help and info page:

<app-link to="not-found">Not Found</app-link> <app-link to="config">Help & 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 & Info</a>

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

<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 & Info</router-link>

<a href="/not-found" class="">Not Found</a> <a href="/config" class="router-link-exact-active router-link-active">Help & Info</a>

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?

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...

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

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

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.

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

sharkykh · 2018-07-04T15:29:42Z

wait, I'm getting console errors on this branch (let me check)

The first argument for `Vue.component` must be the name. It won't register the component without it.

``` vue.js:597 [Vue warn]: Error in render: "TypeError: Cannot read property 'startsWith' of null" found in ---> <AppLink> <Config> <Root> ```

cleanup app-link for tests

21caa76

OmgImAlexis added Enhancement Concluded Needs review labels Jul 2, 2018

OmgImAlexis added this to the 0.2.7 milestone Jul 2, 2018

OmgImAlexis requested a review from sharkykh July 2, 2018 16:12

OmgImAlexis mentioned this pull request Jul 2, 2018

Feature/add vue tests #4527

Merged

sharkykh previously requested changes Jul 2, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

OmgImAlexis added 4 commits July 3, 2018 12:04

remove isAnonymised

2886218

moved template property to top

ab8e534

remove redundant component name

ac325f1

remove MEDUSA from component

d806089

sharkykh reviewed Jul 3, 2018

View reviewed changes

add review code

e313e43

sharkykh previously approved these changes Jul 4, 2018

View reviewed changes

sharkykh added 3 commits July 4, 2018 19:04

Fix component registration

76d140a

The first argument for `Vue.component` must be the name. It won't register the component without it.

indexerName computed prop was missing some code

2de7d08

Fix null computedHref when href is loaded after creation

e13d1af

``` vue.js:597 [Vue warn]: Error in render: "TypeError: Cannot read property 'startsWith' of null" found in ---> <AppLink> <Config> <Root> ```

sharkykh dismissed their stale review via e13d1af July 4, 2018 16:19

sharkykh approved these changes Jul 4, 2018

View reviewed changes

OmgImAlexis merged commit d67c588 into develop Jul 5, 2018

OmgImAlexis deleted the feature/cleanup-app-link branch July 5, 2018 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup app-link for tests #4526

cleanup app-link for tests #4526

OmgImAlexis commented Jul 2, 2018 •

edited

Loading

sharkykh Jul 2, 2018

OmgImAlexis Jul 2, 2018

sharkykh Jul 2, 2018 •

edited

Loading

OmgImAlexis Jul 2, 2018

sharkykh Jul 2, 2018

sharkykh Jul 2, 2018

sharkykh Jul 2, 2018

sharkykh Jul 2, 2018

OmgImAlexis Jul 2, 2018

sharkykh Jul 2, 2018

OmgImAlexis Jul 2, 2018

sharkykh Jul 2, 2018 •

edited

Loading

sharkykh Jul 2, 2018

OmgImAlexis Jul 2, 2018

This comment has been minimized.

sharkykh Jul 3, 2018 •

edited

Loading

sharkykh Jul 4, 2018

OmgImAlexis Jul 4, 2018

sharkykh Jul 4, 2018

sharkykh Jul 4, 2018 •

edited

Loading

OmgImAlexis Jul 4, 2018 •

edited

Loading

sharkykh Jul 4, 2018

OmgImAlexis Jul 4, 2018

OmgImAlexis Jul 4, 2018

sharkykh Jul 4, 2018

sharkykh Jul 4, 2018

sharkykh commented Jul 4, 2018 •

edited

Loading

cleanup app-link for tests #4526

cleanup app-link for tests #4526

Conversation

OmgImAlexis commented Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkykh Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkykh Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

sharkykh Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkykh Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

OmgImAlexis Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkykh commented Jul 4, 2018 • edited Loading

OmgImAlexis commented Jul 2, 2018 •

edited

Loading

sharkykh Jul 2, 2018 •

edited

Loading

sharkykh Jul 2, 2018 •

edited

Loading

sharkykh Jul 3, 2018 •

edited

Loading

sharkykh Jul 4, 2018 •

edited

Loading

OmgImAlexis Jul 4, 2018 •

edited

Loading

sharkykh commented Jul 4, 2018 •

edited

Loading