MBS-9606: Rewrite MusicBrainz ISRC index page in React/JSX #603
Conversation
|
Depends on PR #599. |
mwiencek
force-pushed
the
isrc-pages-react
branch
2 times, most recently
from
c6920da
to
ce53bfb
Compare
|
The page couldn't load for me (it would throw an exception right away) and was broken in various ways. I pushed some fixes and added some Flow types. I think it's in a mergeable state now. The types create conflicts with #608 but I intend to fix those myself |
| $json->{gid} = $self->gid; | ||
| $json->{name} = $self->name; | ||
| $json->{unaccented_name} = $self->unaccented_name; | ||
| return $json; |
There was a problem hiding this comment.
Can we make this and the other subs like so, just to maintain consistency? I also feel the below syntax is much more concise.
around TO_JSON => sub {
my ($orig, $self) = @_;
return {
%{$self->$orig},
gid => $self->gid,
name => $self->name,
unaccented_name => $self->unaccented_name
};
};
There was a problem hiding this comment.
Okay. I only used the current method because it's faster (about 2.7x with perl 5.26 on my macbook; it doesn't have to allocate a new hash and then merge the hashes) and these TO_JSON methods have to be called hundreds or thousands of times on each page load. But I didn't do a proper benchmark so it may be negligible once you take the slow Moose method calls into account.
There was a problem hiding this comment.
I don't mind either actually. I just saw a lot of the code wrapping the TO_JSON as I quoted, so thought I'd point it out. If the PR version is indeed faster then maybe we'd want to keep that and switch newer code to it too.
There was a problem hiding this comment.
Well, I guess I'll keep what I had since I'm lazy. :) But it was probably premature optimization. Moose is slow, and the slowness of the around modifiers probably dwarfs the hash allocations.
| @@ -1,111 +1,120 @@ | |||
| /* | |||
| * @flow | |||
| * Copyright (C) 2015 MetaBrainz Foundation | |||
There was a problem hiding this comment.
We can update the headers to 2018?
There was a problem hiding this comment.
It's supposed to be the year of first publication, so I'm not sure it makes sense to update it unless significant changes are made.
mwiencek
force-pushed
the
isrc-pages-react
branch
from
ce53bfb
to
99f2c50
Compare
|
Force-pushed some tweaks to root/types.js to avoid conflicts with #608 |
The non-superficial reason for this change is that it makes sorting the keys easier (via $your_text_editors_sort_feature) if they all have consistent quoting. The superficial reason is that it looks nicer.
In order to allow linking these entities via the entityHref utility.
It now just accepts the entity directly and figures out the type/id for you. The previous version was more flexible, but I don't think we need it to be.
mwiencek
force-pushed
the
isrc-pages-react
branch
from
99f2c50
to
145a2b7
Compare
Some classes that extend Entity have a name subroutine but don't want it serialized, e.g. ISRC/ISWC. Perhaps those subroutines should be removed, but this makes the code easier to reason about in any case.
Fixes the usage in MergeHelper
mwiencek
force-pushed
the
isrc-pages-react
branch
from
145a2b7
to
c12b8f1
Compare
Replaced all the isrc/index.tt file with an equivalent template written in React/JSX. Accepted on Google Code-In by yvanzo.