Support Refactor #1170

skipjack · 2017-04-29T04:37:26Z

Re-implementing the reverted refactor/fix of the Support component.

@bebraw moving the conversation from #1168 to here, my attempts so far have failed. Please let me know any suggestions, if we don't get this in somehow we'll likely have to re-open #1167 and #411.

bebraw · 2017-04-29T17:14:39Z

That fetch doesn't feel right. I wonder if it would be possible to push the problem outside of React and manage it on build like we do for loaders/plugins/etc.

skipjack · 2017-04-29T17:50:45Z

Ah like generate a json file on every build cycle that contains the backers/sponsors data?

bebraw · 2017-04-29T18:04:50Z

Yeah, it doesn't have to be more complex than that. Also avoids some requests for the client.

skipjack · 2017-04-29T18:10:29Z

I'll update with that change and re-test.

…rom opencollective Fetching the actual JSON containing our supporters gives us more flexibility to tackle issues like #411. It also give a model to follow for our, now extracted, additional sponsors list (those not from open collective).

This includes an attempt at a server-side fetch although not sure it worked.

Integrating opencollective sponsor/backer retrieval into the fetch the process which saves some client side requests and fixes the build.

skipjack · 2017-04-29T21:23:09Z

@bebraw just moved it to the npm run fetch process. Please take a look when you have a chance to make sure everything is sound.

skipjack · 2017-04-29T22:35:37Z

@bebraw we should definitely squash and merge this one, once you've reviewed.

bebraw · 2017-05-01T08:01:59Z

package.json

@@ -98,6 +98,7 @@
    "yaml-frontmatter-loader": "0.0.3"
  },
  "dependencies": {
+    "isomorphic-fetch": "^2.2.1",


Please drop this as the implementation doesn't need it anymore.

Good catch.

skipjack · 2017-05-01T14:41:11Z

@bebraw should be good to go now.

bebraw · 2017-05-01T15:01:49Z

components/support/support.jsx

@@ -1,56 +1,50 @@
 import React from 'react';
+// import PropTypes from 'prop-types';
+import Additional from './support-additional.json';
+import 'isomorphic-fetch';


No need for import 'isomorphic-fetch';.

ah right again!

skipjack · 2017-05-01T16:33:26Z

@bebraw ok now it should be good to go (hopefully 😁 )

bebraw · 2017-05-01T16:34:49Z

components/support/support-additional.json

@@ -0,0 +1,18 @@
+[
+  {
+    "id": 20000,


Where do the ids come from? Is there any chance these could clash in the future?

Where do the ids come from?

Hardcoded as I was trying to stay consistent with the opencollective data. And they're used for the key prop.

Is there any chance these could clash in the future?

Potentially, yes. I tried to bump them pretty high to avoid that.

In general I like to avoid key={ index } everywhere, hence the ids. In our case, it currently doesn't matter too much for non-interactive components as a render() will never be called again on the client side. I'm ok with key={ supporter.id || index } for now and maybe shifting back to something like this, except using higher ids or uuids to prevent collisions, down the road.

bebraw · 2017-05-01T16:35:26Z

components/support/support-additional.json

+    "name": "MoonMail",
+    "username": "moonmail",
+    "tier": "sponsor",
+    "avatar": "https://static.moonmail.io/moonmail-logo.svg",


Would it be better to host the svgs on webpack site instead? I can see pros/cons to both and we can stick with the current solution.

Idk, I kind of like keeping these external for now. I think our asset management still has some room for improvement so I'd like to avoid adding any more than there already are for now. Plus, this stays consistent with the rest of the supporter images which are also fetched externally from the opencollective urls.

bebraw · 2017-05-01T16:35:54Z

components/support/support.jsx

@@ -1,56 +1,49 @@
 import React from 'react';
+// import PropTypes from 'prop-types';


Maybe drop the commented bits?

Sure... I was leaving it in for now as kind of a TODO. I'd like to add prop type checking to some of our components at some point but I'm fine with curbing this for now.

skipjack · 2017-05-01T22:05:44Z

@bebraw pushed some more tweaks based on your comments.

bebraw

Good to go, I can't find anything else. 👍

@bebraw

cc @bebraw

…ebpack/webpack#3683

skipjack · 2017-05-02T13:53:26Z

Seeing the same error now as on #1177... @bebraw any idea what's going on there?

bebraw · 2017-05-02T14:04:49Z

failure.diag can be undefined at scripts/check-links.js apparently. I think you have to add a check against that. I'm not sure if that fixes everything, though.

skipjack · 2017-05-02T21:08:40Z

Yep, debugging the issue now... hopefully once this is resolved we can get this PR (and the other) merged finally. I'm also going to remove that --exclude as the support urls have changed now that we're pulling in the raw data.

skipjack · 2017-05-02T21:39:52Z

@Munter any idea what the FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory is about? Seems to be getting thrown from somewhere within hyperlink.

Munter · 2017-05-03T09:00:05Z

@skipjack Haven't seen that one before. But since hyperlink is not particularly clever about unloading the assets it has visited and out of memory error seems probable. @papandreou is this specific error known to you?

papandreou · 2017-05-03T09:06:53Z

Yeah, that's how V8 dies when the JavaScript heap exceeds the maximum allowed size (even after performing a full GC). The maximum size is specified using the node --max-old-space-size=<numMegaBytes> switch and defaults to 2000 (2 GB).

That could either indicate that the combined size of the assets that hyperlink is loading has increased so it doesn't fit anymore, or that there is a bug that means it'll run off and follow external relations/redirects infinitely.

As the first thing I would try invoking it as node --max-old-space-size=5000 hyperlink ... to rule out the latter.

Munter · 2017-05-03T09:46:01Z

There's a bunch of things that hyperlink could do to free up memory during the run. Once an asset has been visited it's never used again. So I could potentially unload it. Similarly I might be able to avoid loading assets that are known to not contain external relations and are huge, like videos.

skipjack requested a review from bebraw April 29, 2017 04:37

skipjack added Bug UI/UX labels Apr 29, 2017

skipjack added 2 commits April 29, 2017 16:00

refactor: a few more fixes for the Support component

91c18e5

This includes an attempt at a server-side fetch although not sure it worked.

skipjack force-pushed the support-refactor branch from f36db08 to 91c18e5 Compare April 29, 2017 20:00

refactor: clean up fetch script and add sponsor/backer fetch script

c05c1a9

Integrating opencollective sponsor/backer retrieval into the fetch the process which saves some client side requests and fixes the build.

whoops

86cac27

bebraw suggested changes May 1, 2017

View reviewed changes

refactor: remove obsolete dep

3990bd3

bebraw reviewed May 1, 2017

View reviewed changes

refactor: remove unnecessary import

5d6b763

bebraw reviewed May 1, 2017

View reviewed changes

refactor: minor tweaks to the Support component

6e8995d

bebraw approved these changes May 2, 2017

View reviewed changes

skipjack added 3 commits May 2, 2017 09:06

fix(links): fix relative links that work in dev but fail in production

11f2b64

cc @bebraw

fix(links): fix incorrect link in source-map-devtool-plugin

9126404

docs(plugins): remove AbstractPlugin reference as it was removed in w…

f8daba2

…ebpack/webpack#3683

fix(tests): fix link-linting and remove the now unnecessary --exclude

4cfd178

refactor: add missing semicolons

27c4e73

skipjack merged commit 3513cb5 into master May 2, 2017

skipjack deleted the support-refactor branch May 2, 2017 23:07

skipjack mentioned this pull request May 3, 2017

DllPlugin & DllReferencePlugin docs (without pending examples) #1177

Merged

skipjack mentioned this pull request May 5, 2017

Improve lint:links - exclude support images/links #1194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Refactor #1170

Support Refactor #1170

skipjack commented Apr 29, 2017 •

edited

Loading

bebraw commented Apr 29, 2017

skipjack commented Apr 29, 2017 •

edited

Loading

bebraw commented Apr 29, 2017

skipjack commented Apr 29, 2017

skipjack commented Apr 29, 2017 •

edited

Loading

skipjack commented Apr 29, 2017 •

edited

Loading

bebraw May 1, 2017

skipjack May 1, 2017

skipjack commented May 1, 2017

bebraw May 1, 2017

skipjack May 1, 2017

skipjack commented May 1, 2017

bebraw May 1, 2017

skipjack May 1, 2017

bebraw May 1, 2017

skipjack May 1, 2017

bebraw May 1, 2017

skipjack May 1, 2017

skipjack commented May 1, 2017

bebraw left a comment

skipjack commented May 2, 2017

bebraw commented May 2, 2017

skipjack commented May 2, 2017

skipjack commented May 2, 2017

Munter commented May 3, 2017

papandreou commented May 3, 2017

Munter commented May 3, 2017

		@@ -1,56 +1,49 @@
		import React from 'react';
		// import PropTypes from 'prop-types';

Support Refactor #1170

Support Refactor #1170

Conversation

skipjack commented Apr 29, 2017 • edited Loading

bebraw commented Apr 29, 2017

skipjack commented Apr 29, 2017 • edited Loading

bebraw commented Apr 29, 2017

skipjack commented Apr 29, 2017

skipjack commented Apr 29, 2017 • edited Loading

skipjack commented Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipjack commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipjack commented May 1, 2017

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

skipjack commented May 1, 2017

bebraw left a comment

Choose a reason for hiding this comment

skipjack commented May 2, 2017

bebraw commented May 2, 2017

skipjack commented May 2, 2017

skipjack commented May 2, 2017

Munter commented May 3, 2017

papandreou commented May 3, 2017

Munter commented May 3, 2017

skipjack commented Apr 29, 2017 •

edited

Loading

skipjack commented Apr 29, 2017 •

edited

Loading

skipjack commented Apr 29, 2017 •

edited

Loading

skipjack commented Apr 29, 2017 •

edited

Loading