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

Plural vs. singular for certain core module names that aren't piece types in A3 #2261

Closed
boutell opened this issue Jul 22, 2020 · 8 comments
Closed

Comments

@boutell
Copy link
Member

boutell commented Jul 22, 2020

Question about our upcoming renaming plans. We said we would give all modules singular names. Some examples that make me think follow.

  • apos.areas: this is a module that is in charge of all areas, but that excuse isn’t good enough for a pieces module to be plural, so it’s not good enough here. So it becomes apos.area. Or I’m going crazy. Sure, fine.

There is no conflict with inserting an area in a template because that is {% area %} now. However it does mean that we might have to find another way of linting for projects with legacy 2.x apos.area calls in them. Because we can't just replace apos.area with a helpful warning function if it is the name of a module. At least they will get an error ("not a function").

  • apos.utils: this literally contains more than one utility. “Literally contains” is @localghost443 's suggested trigger for a plural. But, nodejs itself has a util module, which I’ve often found confusing because apostrophe contains a utils module. Do we rename our module to match?

  • apos.caches: in charge of all caches - yes, yes, not good enough - but the syntax is gonna be weird if we make it singular. apos.cache.get really, really sounds like it gets a value out of a cache! But it doesn't. It gets you a cache, that you can get values from.

OK, big 👾 🧠 moment: I propose we fix this forever like so...

// Remember an oembed response from youtube
await apos.cache.set('oembed', 'https://youtube.com/v/something', { data... });
// Get it back later
await apos.cache.get('oembed', 'https://youtube.com/v/something');

We kill the idea of multiple cache objects as part of the API completely, and just always include a namespace name when getting and setting things. Having multiple caches, one per namespace, as part of the implementation under the hood depends on how the cache is implemented but an apostrophe developer should not care. They should have a simple, normal apostrophe module method they can call to get something. This also fixes our documentation about how to use the module, which is currently written in a weird way because of the "call get to get a cache object" issue.

Can I get a hell yeah?

  • I have been over the modules list in core and I think these are the only ones I'm unsure about.

cc @stuartromanek @abea @falkodev

@stuartromanek
Copy link
Member

  • apos.area
  • 🤖 apos.utils (no opinion on being matchy matchy w Node)
  • 🧠 apos.cache (having used the caches module a few times i need to always re-read that bit of primer)

@boutell
Copy link
Member Author

boutell commented Jul 22, 2020

@stuartromanek it sounds like you like the proposed change to remove the extra layer of objects from apos.cache and just include the namespace in each call to get or set something?

@abea
Copy link
Contributor

abea commented Jul 22, 2020

  • apos.area ✅
  • apos.utils ✅
  • apos.cache (namespaced to get cache value) ✅

@stuartromanek
Copy link
Member

@boutell yes, I'm in favor of the proposed changes to apos.cache's methods

@thefrana
Copy link

  • apos.area
  • apos.util (I like to keep things simple. If we already decided that every module name will be singular then keep this one also singular. Plus you get similarity with Node)
  • apos.cache (I am not 100% sure, but I would go for proposed changes)

@falkodev
Copy link
Contributor

falkodev commented Jul 23, 2020

  • apos.area ✅
  • apos.cache ✅
  • apos.util:
    • the argument to have it plural can be applied to the other 2 categories here
    • you get only one "util" at a time anyway => apos.util.generateId()
    • I like the fact Node uses it as a singular too

Sorry to be a pain 😬  I won't fight if everyone else prefers the plural form of util 😄

@abea
Copy link
Contributor

abea commented Jul 23, 2020

The singular util points are solid. I don't feel strongly on that.

@boutell
Copy link
Member Author

boutell commented Jul 23, 2020

I think given that everything else will be singular (no module other than utils comes close to Ben's criteria for a plural), and that Node.js uses the singular for its core util module which is in a very similar spirit and used by most node developers, we should go with all singular. Looks like we have a consensus here!

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

No branches or pull requests

5 participants