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

feature 262: refactor facts to uncouple from templating/blaze. #9629

Merged
merged 3 commits into from Feb 21, 2018

Conversation

fgm
Copy link
Contributor

@fgm fgm commented Feb 2, 2018

As envisioned on meteor/meteor-feature-requests#262 :

  • extract logic from facts to separate facts-base and facts-ui packages to avoid the templating dependency when not using the UI part
  • keep the facts package as a legacy compatibility wrapper to avoid breaking existing code

@StorytellerCZ
Copy link
Collaborator

Wouldn't just having a weak dependency on Templating be easier than having two packages?

@fgm
Copy link
Contributor Author

fgm commented Feb 2, 2018

AIUI It would not be equivalent: the included client part just does not work if templating is not included, but it is still being exposed by the package, although it doesn't work in that case, and it still present in the bundle.

By having separate packages, the 100% templating-related client part doesn't get to be included at all. Or am I missing something (entirely possible!) ?

@fgm fgm force-pushed the feature-262_refactor-facts branch from 8b33766 to ccd886d Compare February 2, 2018 16:44
@fgm
Copy link
Contributor Author

fgm commented Feb 2, 2018

This now works in both client and server in a test application. A question bothers me, though: userIdFilter takes a userId argument, but ignores it, instead just relying on the presence of autopublish to decide whether or not to publish this manually maintained (vs MongoDB) collection.

Is this expected ? Should it be modified to account for better conditions than autopublish ? Or, if this is to be maintained for compatibility, shouldn't it be made more flexible to expose the publication to any server-side code wishing to decide upon publication conditions itself ? And the userId parameter removed ?

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @fgm! I've made a few comments inline, but have one additional item to mention. I think it makes sense to deprecate the facts package (see my comments), which means we should consider updating other packages that refer to it. Would you mind:

  • Updating the ddp-server package to use facts-base
  • Updating the mongo package to use facts-base

Thanks again!

@@ -0,0 +1,75 @@
import { Facts, FACTS_COLLECTION, FACTS_PUBLICATION } from './facts_base_both';

console.log('Facts base server');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra console.log.

Copy link
Contributor Author

@fgm fgm Feb 9, 2018

Choose a reason for hiding this comment

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

Done

api.use('ddp', 'server', {unordered: true});

api.mainModule('facts_base_server.js', 'server');
api.mainModule('facts_base_both.js', 'client');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename facts_base_both.js (and all references) to something a little more open ended? "Both" refers to 2 items, which in this case sort of works semantically, because of client/server. "Both" might not always make sense however, so to future proof maybe use something like facts_base_common.js (or even just common.js).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,21 @@
Package.describe({
summary: "Publish internal app statistics",
version: '0.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a new package, let's start out at 1.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

api.mainModule('facts_base_server.js', 'server');
api.mainModule('facts_base_both.js', 'client');

api.export(['Facts', 'FACTS_COLLECTION', 'FACTS_PUBLICATION']);
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to export FACTS_COLLECTION and FACTS_PUBLICATION here. The api.mainModule('facts_base_both.js', 'client'); call will take care of making these available to other packages, via import { FACTS_COLLECTION, FACTS_PUBLICATION } from 'meteor/facts-base';.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,34 @@
import { Facts, FACTS_COLLECTION, FACTS_PUBLICATION } from 'meteor/facts-base';

console.log('Facts UI client');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this extra console.log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,17 @@
Package.describe({
summary: "Display internal app statistics",
version: '0.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

New package, so let's start at 1.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -1,21 +1,12 @@
Package.describe({
summary: "Publish internal app statistics",
version: '1.0.9'
version: '1.0.10'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that instead of bumping the version here and publishing a new version of the soon to be deprecated facts package, that we should leave the facts code and package.js as is. We can add the deprecated notice to the README.md like you have, but we should then also move this entire package into the packages/deprecated directory. That way we can avoid making extra code changes here (and avoid publishing those changes) just to end up deprecating this package anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// By default, we publish facts to no user if autopublish is off, and to all
// users if autopublish is on.
let userIdFilter = function (userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I see what you mean about userId (as mentioned in #9629 (comment)). It shouldn't be needed, so please feel free to remove it. This code hasn't been touched in 5 years, so it just looks like this was never noticed. As for making this more flexible (instead of just checking for Package.autopublish), if you're interested in improving this, please feel free (as long as the existing functionality of checking for Package.autopublish is preserved in some way, maybe as a default or fallback, for backwards compatibility).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is best to just leave it untouched for this PR since that would be an API feature change, while this PR is just about splitting code to remove dependencies. Can always be done once this gets in, without delaying for a different API design.

const packageFacts = factsByPackage[pkg];
if (!_.has(packageFacts, fact)) {
factsByPackage[pkg][fact] = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding these braces! 🙂

@fgm fgm force-pushed the feature-262_refactor-facts branch 2 times, most recently from 9bc5a65 to 42a97de Compare February 9, 2018 10:42
@fgm fgm changed the title [WIP] feature 262: refactor facts to uncouple from templating/blaze. feature 262: refactor facts to uncouple from templating/blaze. Feb 9, 2018
@fgm
Copy link
Contributor Author

fgm commented Feb 9, 2018

Looks good to me and works in my test application (client and server). I haven't touched the userIdFilter() part for now, as I think it needs to be given more thought.

@fgm
Copy link
Contributor Author

fgm commented Feb 14, 2018

@hwillson just pinging to make sure you're aware the PR is ready to review.

@hwillson
Copy link
Contributor

Sorry for the delay @fgm - I've scheduled some time for PR reviews later today, so I'll get back to this shortly. Thanks!

@fgm fgm force-pushed the feature-262_refactor-facts branch from 42a97de to 33f819c Compare February 14, 2018 14:04
@fgm
Copy link
Contributor Author

fgm commented Feb 14, 2018

Just rebased it and added the PR reference in History.md, BTW.

@hwillson
Copy link
Contributor

Changes look great @fgm - thanks for working on this!

@benjamn benjamn merged commit fd63390 into meteor:devel Feb 21, 2018
@benjamn benjamn added this to the Package Patches milestone Feb 21, 2018
@fgm fgm deleted the feature-262_refactor-facts branch February 21, 2018 17:33
@engrpeters
Copy link

How do I make use the fact package?

@fgm
Copy link
Contributor Author

fgm commented Nov 2, 2018

@engrpeters the doc issue for this package is meteor/docs#33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants