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.

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

Move loading logic into each feature #1694

Merged
merged 22 commits into from Jan 15, 2019
Merged

Move loading logic into each feature #1694

merged 22 commits into from Jan 15, 2019

Conversation

fregante
Copy link
Member

@fregante fregante commented Jan 4, 2019

  • Move loading logic (when, where) near the feature's code instead of being listed in 3 separate functions (init, onDomReady, ajaxedPagesHandler) in a different file (content.js)
  • Avoids the messy content.js list of imports/uselessFunctionNames by halving the file length
  • Enables us to actually list all the features in Options (not part of this PR) since they are all added in feature.add('feature-name') on load

This is just a proof of concept so only a feature has been implemented.

@fregante fregante added meta Related to Refined GitHub itself under discussion labels Jan 4, 2019
@fregante fregante self-assigned this Jan 4, 2019
@sindresorhus
Copy link
Member

👍 This is a great idea.

source/features/useful-not-found-page.js Outdated Show resolved Hide resolved
@fregante
Copy link
Member Author

fregante commented Jan 7, 2019

Alright, big change. 🎉 I solved all the bugs I noticed for now, but I'm sure there's a few I didn't notice (out of the 83 javascript features 😰)

Need some reviewers to try this together for a few days and then merging it asap, give the overarching patch.

Pinging collaborators and recent contributors: @notlmn @jerone @dertieran @leggsimon @tanmayrajani @fxedel @yakov116

@sindresorhus
Copy link
Member

Everyone: Please don’t commit any changes to master until this PR is merged.

source/libs/features.js Outdated Show resolved Hide resolved
@dertieran

This comment has been minimized.

@fregante
Copy link
Member Author

fregante commented Jan 7, 2019

@dertieran feature order could just be defined as dependencies in the features.add object if someone built an "import all" webpack plugin. A separate PR can be sent by someone interested in doing implementing that.

For now, even the dependency thing sounds complicated (it must check the validity, remember the order, run when all the features are ready, and run at every run() call)

@sindresorhus

This comment has been minimized.

safeOnAjaxedPages -> onAjaxedPages
onAjaxedPages -> onAjaxedPagesRaw
@dertieran

This comment has been minimized.

busches
busches previously requested changes Jan 7, 2019
source/content.js Outdated Show resolved Hide resolved
source/content.js Outdated Show resolved Hide resolved
@sindresorhus

This comment has been minimized.

busches and others added 2 commits January 8, 2019 09:43
Co-Authored-By: bfred-it <github@bfred.it>
Co-Authored-By: bfred-it <github@bfred.it>
@leggsimon

This comment has been minimized.

@fregante

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@fregante

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@fregante

This comment has been minimized.

@jerone
Copy link
Contributor

jerone commented Jan 14, 2019

Last call for reviews. 😄

I finally had time to test it, and the features I use are still working. 👍

Copy link
Member

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

Works well now 👌

@fregante
Copy link
Member Author

I'll merge in ~14 hours to allow for more testing.

Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

Works well here too

Copy link
Contributor

@leggsimon leggsimon left a comment

Choose a reason for hiding this comment

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

Noticed a small edge case but not sure if it needs a fix. Out of curiousity though I noticed that refined-github only works if you’re signed in. Is that necessary? I assume this is because some features require authenticated API calls but I wonder whether the features that require a signed in user should define that in includes

eg.

features.add({
	id: 'show-user-top-repositories',
	include: [
		features.isUserProfile,
+		features.userIsSignedIn
	],
	load: features.onAjaxedPages,
	init
});

source/libs/features.js Show resolved Hide resolved
@sindresorhus
Copy link
Member

Noticed a small edge case but not sure if it needs a fix. Out of curiousity though I noticed that refined-github only works if you’re signed in. Is that necessary?

We're not interested in supporting logged-out state. Refined GitHub should not run at all if the user is logged out.

@fregante fregante merged commit 7ec5717 into master Jan 15, 2019
@fregante fregante deleted the self-contained-features branch January 15, 2019 17:04
@fregante
Copy link
Member Author

🎉

Thanks all for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to Refined GitHub itself under discussion
Development

Successfully merging this pull request may close these issues.

None yet

8 participants