Skip to content

Move loading logic into each feature #1694

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

Jump to bottom
Merged
fregante merged 22 commits into from
Jan 15, 2019
Merged

Move loading logic into each feature #1694

fregante merged 22 commits into from
Jan 15, 2019

Conversation

@fregante
Copy link
Member

@fregante fregante commented Jan 4, 2019 •
edited
Loading

  • 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
@fregante fregante requested a review from sindresorhus January 4, 2019 09:32
@sindresorhus
Copy link
Member

sindresorhus commented Jan 4, 2019

👍 This is a great idea.

sindresorhus
sindresorhus previously requested changes Jan 4, 2019
View reviewed changes
@fregante fregante force-pushed the self-contained-features branch from 08d2d09 to 4a7493e Compare January 7, 2019 09:25
@fregante
Copy link
Member Author

fregante commented Jan 7, 2019 •
edited
Loading

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

sindresorhus commented Jan 7, 2019

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

jerone
jerone reviewed Jan 7, 2019
View reviewed changes
jerone
jerone reviewed Jan 7, 2019
View reviewed changes
jerone
jerone reviewed Jan 7, 2019
View reviewed changes
notlmn
notlmn reviewed Jan 7, 2019
View reviewed changes
@dertieran

This comment has been minimized.

Sign in to view
@fregante
Copy link
Member Author

fregante commented Jan 7, 2019 •
edited
Loading

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

Sign in to view
@fregante
Renamed loading functions
4833832 
safeOnAjaxedPages -> onAjaxedPages
onAjaxedPages -> onAjaxedPagesRaw
@fregante
Error if definition props are invalid
4e9e27c
@dertieran

This comment has been minimized.

Sign in to view
busches
busches previously requested changes Jan 7, 2019
View reviewed changes
@sindresorhus

This comment has been minimized.

Sign in to view
busches and others added 2 commits January 8, 2019 09:43
@busches @fregante
Renamed loading functions /2
8b71d18 
Co-Authored-By: bfred-it <github@bfred.it>
@busches @fregante
Renamed loading functions /3
74cd801 
Co-Authored-By: bfred-it <github@bfred.it>
@leggsimon

This comment has been minimized.

Sign in to view
@fregante

This comment has been minimized.

Sign in to view
@sindresorhus

This comment has been minimized.

Sign in to view
@fregante

This comment has been minimized.

Sign in to view
@sindresorhus

This comment has been minimized.

Sign in to view
@fregante fregante mentioned this pull request Jan 14, 2019
Closed
@fregante

This comment has been minimized.

Sign in to view
@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. 👍

@dertieran dertieran mentioned this pull request Jan 14, 2019
Closed
sindresorhus
sindresorhus approved these changes Jan 15, 2019
View reviewed changes
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

fregante commented Jan 15, 2019

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

yakov116
yakov116 approved these changes Jan 15, 2019
View reviewed changes
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

leggsimon
leggsimon reviewed Jan 15, 2019
View reviewed changes
Copy link
Contributor

@leggsimon leggsimon left a comment •
edited
Loading

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
});

@sindresorhus
Copy link
Member

sindresorhus commented Jan 15, 2019

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

fregante commented Jan 15, 2019

🎉

Thanks all for the reviews!

fregante added a commit to jerone/refined-github that referenced this pull request Jan 15, 2019
@fregante
Use updated features API from refined-github#1694
d87e2f3
This was referenced Jan 16, 2019
Merged
Closed
Closed
@notlmn notlmn mentioned this pull request May 28, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sindresorhus sindresorhus sindresorhus approved these changes

@yakov116 yakov116 yakov116 approved these changes

@busches busches busches left review comments

@hkdobrev hkdobrev Awaiting requested review from hkdobrev

+4 more reviewers

@dertieran dertieran dertieran left review comments

@leggsimon leggsimon leggsimon left review comments

@notlmn notlmn notlmn left review comments

@jerone jerone jerone approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fregante fregante

Labels

meta Related to Refined GitHub itself under discussion

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@fregante @sindresorhus @dertieran @leggsimon @jerone @busches @yakov116 @notlmn