module inheritance (moog) is difficult to master, developers don't know where to put their code in modules

[boutell]

Here is the planned replacement for the current index.js syntax for modules. This is not backwards compatible, it's the 3.0 format. However a conversion tool is under construction and has already been used to help convert apostrophe core itself.

In general, we are deprecating the imperative, "build the module by calling stuff" style and replacing it with a declarative style, while avoiding technical terms and invented language.

// lib/modules/shoes/index.js, a pieces subclass

module.exports = {

  extends: 'apostrophe-pieces',

  // "improve" would also go at this level

  // set up schema fields. If there are subclasses with fields they will
  // merge sensibly with what is done here.

  // fields may just be an object if you don't have any conditional fields to add, or not add, based
  // on options

  fields(self, options) => ({
    add: {
      shoeSize: {
        type: 'integer',
        label: 'Shoe Size'
      },
      price: {
        type: 'float'
      },
      // ES2015 makes option-dependent fields easy
      ...(options.specialField ? {
        special: {
          type: 'whatever'
        }
      } : {})
    },
    remove: [ 'tags' ],
    groups: {
      shoes: {
        label: 'Shoes',
        fields: [ 'shoeSize' ]
      },
      // The "utility rail" at right which appears no matter what tab is active.
      // Overuse of this space is discouraged
      utility: {
        fields: [ 'title', 'slug' ]
      }
    }
  }),

  options: {
    // "Plain old options" now go in their own distinct section. They
    // override straightforwardly in subclasses
    searchable: false
    // "name" option for pieces is dead, defaults to module name
    // as it should have in the first place, sorry
  },

  async init(self, options) {
    // options, fields and methods are ready for use here
    await self.connectToShoestoreBackend();
  },

  async afterAllSections(self, options) {
    // You'll probably never write one of these! But if you were adding support for an
    // entirely new section of module config (like routes or methods, but something
    // else) you might need to run code at this point to attach the routes to express, etc.
    // "init" has already resolved at this point and all of the sections have been attached
    // to `self`.
  },

  methods: (self, options) => ({
    async fetchMonkeys(req, doc) {
      ... code for this method ...
      // having self in scope is key here and in pretty much all other sections
      // containing functions
      self.doSomething();
    },
    doThing() {
      ... code for this method ...
    },
    doOtherThing() {
      ... code for this method ...
    },
    // Middleware we'll add selectively to certain routes
    requireAdmin(req, res, next) {
      if (!self.apos.permissions.can('admin')) {
        return res.status(403).send('forbidden');
      }
    }
  }),

  extendMethods: (self, options) => ({
    async adjustHovercraft(_super, req, doc, options) {
      await _super(req, doc, options);
      doSomethingMore();
    }
  }),

  handlers: (self, options) => ({
    'apostrophe-pages:beforeSend': {
      // Named so they can be extended
      async addPopularProducts(req) { ... },
      async addBoringBooks(req) { ... }
    }
  }),

  extendHandlers: (self, options) => ({
    'apostrophe-pages:beforeSend': {
      // Inherited from base class, we can use _super to
      // invoke the original and do more work
      async addNavigation(_super, req) { ... }
    }
  }),

  helpers: (self, options) => ({
    includes(arr, item) {
      return arr.includes(item);
    }
  }),

  extendHelpers: (self, options) => ({
    // Extend a base class helper called colorCode with a new default
    colorCode(_super, item) {
      const color = _super(item);
      return color || 'red';
    }
  }),

  // middleware registered in the middleware block runs on ALL requests
  middleware(self, options) => ({
    ensureData(req, res, next) {
      req.data = req.data || {};
      return next();
    }),
    // This middleware needs to run before that provided by another module,
    // so we need to pass an option as well as a function
    reallyEarlyThing: {
      before: 'other-module-name',
      middleware: (req, res, next) => { }
    }
  }),

  // there is no extendMiddleware because the middleware pattern does not lend itself to the
  // "super" pattern

  apiRoutes: (self, options) => ({
    post: {
      // This route needs middleware so we pass an array ending with the route function
      upload: [
        // Pass a method as middleware
        self.requireAdmin,
        // Actual route function
        async (req) => { ... }
      ],
      async insert(req) {
        // No route-specific middleware, but global middleware always applies
      }
      // Leading / turns off automatic css-casing and automatic mapping to a module URL.
      // Presto, public API at a non-Apostrophe-standard URL
      '/track-hit': async (req) => {
        // route becomes /modules/modulename/track-hit, auto-hyphenation so we can
        // use nice camelcase method names to define routes
        return self.apos.docs.db.update({ _id: req.body._id },
          { $inc: { hits: 1 } }
        );
      }
    }
  }),

  extendApiRoutes: (self, options) => ({
    post: {
      // insert route is inherited from base class, let's
      // extend the functionality
      async insert(_super, req) {
        await _super(req);
        // Now do something more...
      }
    }
  }),

  // REST API URLs can be done with `apiRoutes` but they would
  // look a little weird with `apiRoutes` because you
  // would need to specify the empty string as the name of the "get everything"
  // GET route, for instance. So let's provide a separate section
  // to make it less confusing to set them up

  restApiRoutes: (self, options) => ({
    async getAll(req) { returns everything... },
    async getOne(req, id) { returns one thing... },
    async post(req) { inserts one thing via req.body... },
    async put(req, id) { updates one thing via req.body and id... },
    async delete(req, id) { deletes one thing via id... }
    async patch(req, id) { patches one thing via req.body and id... }
  }),
  
  // extendRestApiRoutes works like extendApiRoutes of course

  components: (self, options) => ({
    // In template: {% component 'shoes:brandBrowser' with { color: 'blue' } %}
    async brandBrowser(req, data) {
      // Renders the `brandBrowser.html` template of this module,
      // with `data.brands` available containing the results of this
      // third party API call
      return {
        // Pass on parameter we got from the component tag in the template
        brands: await rq('https://shoebrands.api', { color: data.color })
      };
    }
  }),

  extendComponents: (self, options) => ({
    // Extend a component's behavior, reusing the original to do most of the work 
    async brandBrowser(_super, req, data) {
      const result = await _super(req, data); 
      if (result.color === 'grey') {
        result.color = 'gray';
      }
      return result;
    }
  }),

  // Typically used to adjust `options` before the base class sees it, if you need
  // to do that programmatically in ways the `options` property doesn't allow for.
  // Pretty rare now that `fields` is available

  beforeSuperClass(self, options) {
    // ...
  },

  // Add features to database queries, i.e. the objects returned by self.find() in this module.
  // `self` is the module, `query` is the individual query object
  queries(self, query) {
    return {
      // Query builders. These are chainable methods; they get a chainable setter method for free,
      // you only need to specify what happens when the query is about to execute
      builders: {
        free: {
          finalize() {
            const free = query.get('free');
            const criteria = query.get('criteria');
            query.set('criteria', { $and: [ criteria, { price: free ? 0 : { $gte: 0 } } ] });
          }
        }
      },
      methods: {
        // Return a random object matching the query
        async toRandomObject() {
          const subquery = query.clone();
          const count = await subquery.toCount();
          query.skip(Math.floor(count * Math.random));
          query.limit(1);
          const results = await query.toArray();
          return results[0];
        }
      }
    };
  },
};

Why do we think this is better?

• An explicit self, options function for each section that contains functions provides a scope with access to the
  module. Separating those functions by section is a little wordy, but attempting to merge them in a single function leads
  to significant confusion. For instance, you can't pass a method as the function for a route unless methods are initialized
  first in a separate call. And properties like extend must be sniffable before construction of the object begins, which
  means we can't just export one big function that returns one object, or else we'd have to invoke it twice; the first time it would
  be without a meaningful self or options, leading to obvious potential for bugs.
• methods is a simple and descriptive name, familiar from Vue, which has been very successful in achieving developer acceptance, even though Vue also does not use ES6 classes for not entirely dissimilar reasons. In general, Vue components have been designed with simple and descriptive language wherever possible, and we can learn from that and avoid inside baseball jargon.
• extendMethods is a similar, however here each method's first argument is _super, where _super is a reference to the method we're overriding from a parent class. We now have complete freedom to call _super first, last, or in the middle in our new function. It is much less verbose than our current super pattern. Organizing all of these extensions in extendMethods makes the intent very clear. Note that if you just want to "override" (replace) a method, you declare it in methods and that straight up crushes the inherited method. extendMethods is for scenarios where you need reuse of the original method as part of the new one. We use _super because super is a reserved word.
• handlers and extendHandlers provide similar structure for promise event handlers. Again, these get grouped together, making them easier to find, just like Vue groups together computed, methods, etc. As always, handlers must be named. Handlers for the same event are grouped beneath it. This is loosely similar to Vue lifecycle hooks, but intentionally not identical because Apostrophe involves inheritance, and everything needs to be named uniquely so it can be overridden or extended easily.
• helpers and extendHelpers: you get the idea. For nunjucks helpers.
• apiRoutes and extendApiRoutes: you'd actually be able to add apiRoutes, htmlRoutes and plain old routes. see recent Apostrophe changelogs if this is unfamiliar. Note subproperties separating routes of the same name with different HTTP methods.
• fields: just... just look at it. This clobbers addFields/removeFields with tons of beforeConstruct boilerplate.
• middleware and extendMiddleware: replaces the current expressMiddleware property, which is a bit of a hack, with a clear way to define and activate middleware globally. Note the before option which covers some of the less common but most important uses of expressMiddleware in 2.x. As for middleware that is only used by certain routes, methods are a good way to deliver that, as shown here. Note that methods will completely initialize, from base class through to child class, before routes start to initialize, so they will see the final version of each method.
• init is a common name in other frameworks for a function that runs as soon as the module is fully constructed and ready to support method calls, etc. This replaces the inside-baseball name afterConstruct.
• beforeSuperClass is very explicit and hopefully, finally, clarifies when this function runs: before the base class is constructed. It is the
  only function that runs "bottom to top," i.e. the one in the subclass goes first. We used to call it beforeConstruct which says nothing
  about the timing relative to the base class. It is used to manipulate options before the parent class sees them,
  however most 2.x use cases have been eliminated by the introduction of the fields section.
• queries replaces what people used to put in cursor.js files, specifically addFilters calls as well as custom methods. It obviates the need for a separate moog type for cursors, moog is now used only to instantiate modules and ceases to have its own "brand." queries only makes sense in a module that inherits from apostrophe-doc-type-manager.

"What about breaking a module into multiple files?" Well that's a good question, we do this now and it's a good thing. But, nobody's stopping anyone from using require in here. It would work like it does today, you'd pass in self or self, options to a function in the required file.

[ammein]

From my perspective, only take a first glance, It is sooooo familiar and know how to hands on it. Although , it would be nice to have consistent syntax name. Like before, moog have beforeConstruct, construct and afterConstruct . Same goes to react , they have componentDidMount, componentDidCatch and etc. Also react have special event method such as snapshotBeforeUpdate and others. But still has its own familiar consistent syntax to work on . So, adjustOptions would be a quite confusing method name for adjusting options than using previous method name on beforeConstruct . Above all that, I loveeeeee the new replacement for module inheritance and I notice that extend has been change into extends to maintain syntax consistency across all familiar frameworks. Good job ! 👍

[boutell]

Good points. My concern about keeping beforeConstruct is that it might be clear to experienced apostrophe developers but I doubt that new developers know what it means. adjustOptions is new to us, but clear to newcomers.

…

On Wed, Jun 5, 2019, 7:53 AM Amin Shazrin ***@***.***> wrote: From my perspective, only take a first glance, It is sooooo familiar and know how to hands on it. Although , it would be nice to have consistent syntax name. Like before, moog have beforeConstruct, construct and afterConstruct . Same goes to react , they have componentDidMount, componentDidCatch and etc. Also react have special event method such as snapshotBeforeUpdate and others. But still has its own familiar consistent syntax to work on . So, adjustOptions would be a quite confusing method name for adjusting options than using previous method name on beforeConstruct . Above all that, I loveeeeee the new replacement for module inheritance and I notice that extend has been change into extends to maintain syntax consistency across all familiar frameworks. Good job ! 👍 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1940?email_source=notifications&email_token=AAAH27LBUXJAVD73KGPRFQTPY6SMFA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW7O3GQ#issuecomment-499051930>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAH27I7ZMA6MF55OYMC3Z3PY6SMFANCNFSM4HTK3NIA> .

[ammein]

Agree , using explicit matches syntax name like adjustOptions is best way to catch newcomers and provide clear goals on what devs should do on it. Still, need to be consistent syntax name for better recognition rather than recall .

[boutell]

Another question to answer: should we introduce an options key to separate "plain old options" from options with a special meaning? Right now, for instance, your module can't have a plain old option (self.options.whatever) named extend, because of the effects that extend has on inheritance.

[ammein]

I think the way you pronounce the idea of options key is similar to React state or Vue data , is it ? If that so, yes , i think its a good idea to replace with our 'plain old options' and way more familiar to work with using options : {}. I hope that answers to your question

[boutell]

In conventional Apostrophe, when you write: ``` { extend: 'foo', construct: function(self, options) { ... }, color: 'green' } ``` It's obvious what the first two keys do. The third key becomes `self.options.color`. This is how we're able to configure modules simply in `app.js`. The problem is that as we add more "special" keys in this new proposal, there are more and more chances it could conflict with something someone wants to use as a "plain old option." If we move the "plain old options" like this: ``` { extends: 'foo', startup: function(self, options) { ... }, options: { color: 'green' } } ``` Then there are no restrictions on option names. One consequence is that in "app.js" we would have to write:

…

On Thu, Jun 6, 2019 at 12:15 PM Amin Shazrin ***@***.***> wrote: I think the way you pronounce the idea of options key is similar to React state or Vue data , is it ? If that so, yes , i think its a good idea to replace with our 'plain old options' and way more familiar to work with using options : {}. I hope that answers to your question — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1940?email_source=notifications&email_token=AAAH27L4NTIPHRPBXCMH5NDPZEZ3NA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXDMCUQ#issuecomment-499564882>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAH27OKKJY4A6T6OKIJZH3PZEZ3NANCNFSM4HTK3NIA> .

-- *Thomas Boutell, Chief Software Architect* P'unk Avenue | (215) 755-1330 | punkave.com

[boutell]

... in app.js we would have to write: ``` modules: { 'apostrophe-assets': { options: { lean: true } } } ``` For example. We could also decide that app.js can ONLY set options, and anything else MUST live in an index.js file. But, a lot of people like to put `extend` in app.js because it makes it easier to see what everything is at a glance.

…

On Thu, Jun 6, 2019 at 1:11 PM Tom Boutell ***@***.***> wrote: In conventional Apostrophe, when you write: ``` { extend: 'foo', construct: function(self, options) { ... }, color: 'green' } ``` It's obvious what the first two keys do. The third key becomes `self.options.color`. This is how we're able to configure modules simply in `app.js`. The problem is that as we add more "special" keys in this new proposal, there are more and more chances it could conflict with something someone wants to use as a "plain old option." If we move the "plain old options" like this: ``` { extends: 'foo', startup: function(self, options) { ... }, options: { color: 'green' } } ``` Then there are no restrictions on option names. One consequence is that in "app.js" we would have to write: On Thu, Jun 6, 2019 at 12:15 PM Amin Shazrin ***@***.***> wrote: > I think the way you pronounce the idea of options key is similar to > React state or Vue data , is it ? If that so, yes , i think its a good > idea to replace with our 'plain old options' and way more familiar to work > with using options : {}. I hope that answers to your question > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1940?email_source=notifications&email_token=AAAH27L4NTIPHRPBXCMH5NDPZEZ3NA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXDMCUQ#issuecomment-499564882>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAAH27OKKJY4A6T6OKIJZH3PZEZ3NANCNFSM4HTK3NIA> > . > -- *Thomas Boutell, Chief Software Architect* P'unk Avenue | (215) 755-1330 | punkave.com

-- *Thomas Boutell, Chief Software Architect* P'unk Avenue | (215) 755-1330 | punkave.com

[ammein]

Hmm, I understand the issue. If we set this as default , it will conflict our options setup through all modules. I like the idea of inserting an options key in app.js , anything else must live in index.js . However , maybe we also can make another solution like a module wrapper in react for example. If we make such wrapper acts as new module extension that has new method name called or special property keys like startup , options , adjustOptions and everything else. This could fix the whole issue , right ? Correct me if I'm wrong

[ammein]

You can imagine that a wrapper is similar to React.memo(App) and others. The reason to make it as a wrapper is because we can adjust the way module is structured and much more cleaner than needed to adjust the whole apostrophe core modules. What do you think ?

[boutell]

I think that for 3.0 we could do without the wrapper. If we want to be able to use this syntax in 2.x without a bc break, the wrapper makes a lot of sense.

…

On Thu, Jun 6, 2019 at 2:51 PM Amin Shazrin ***@***.***> wrote: You can imagine that a wrapper is similar to React.memo(App) and others. The reason to make it as a wrapper is because we can adjust the way module is structured are much more cleaner than needed to adjust the whole apostrophe core modules. What do you think ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1940?email_source=notifications&email_token=AAAH27IPTT67UTL6FJ3NMHTPZFMCDA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXDZ25A#issuecomment-499621236>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAH27P5THQZD2EMA7XJV6LPZFMCDANCNFSM4HTK3NIA> .

-- *Thomas Boutell, Chief Software Architect* P'unk Avenue | (215) 755-1330 | punkave.com

[boutell]

Change "override" to "extend" because if you just want to override you can just declare again.

[ammein]

@boutell or maybe change to "super" ? That could be much more convenience, right ? Superpower ! 😅

[boutell]

• Everyone approves of the general idea
• Change handlers to eventHandlers
• Remove async from everything except startup
• Keep imperative APIs available for route, etc. so if (options.api) { self.route... } is still possible when needed
• We approved breaking options out of the "moog" namespace at the top level:

// in index.js
{
  extend: 'apostrophe-pieces',
  methods: ...,
  options: {
    label: 'Product'
  }
}

• For now at least, we won't try to backport this to 2.x, and we don't plan to support the old format in 3.x, but both decisions can be revisited.
• Tom will port an existing 3.x module as an example to post here.

[boutell]

Hmm. Changing it to "super" is tempting, but that sounds like it... is the super function, as seen in other languages... which, it is not. The first argument to each extended method will be super though.

[ammein]

@boutell Yeah, I guess you're right. In other language like Java are using "extends" & "override" kinds of methods. So it would be wise to follow familiar syntax name and consistent.

[boutell]

A new wrinkle: what about middleware? The above syntax has no way of accommodating middleware in, say, an apiRoute or plain vanilla route.

Here is a syntax that does allow middleware. I also included a regular route without middleware to show that the simplified syntax isn't going away.

This is what I'm implementing for now but discussion is welcome.

  apiRoutes: {
    post: {
      getHits: [
        // standard express middleware functions
        self.apos.login.requiredMiddleware,
        self.apos.middleware.uploads,
        async(req) {
          // route fn code goes here for /modules/my-module-name/get-hits
        }
      ],
      trackHit(req) {
        // route fn goes here for /modules/my-module-name/track-hit
      }
    }
  }

What if a middleware method is a method of the same module? Will it exist in time to be passed to apiRoute? The answer is yes, because we construct methods fully (through the entire inheritance tree) before we construct routes.

[boutell]

I changed startup to init because it's just as familiar a word, perhaps even more so for this purpose, and it is already used by Apostrophe (afterInit events for example).

I am merging moog into a3 rather than releasing another separate npm version of moog, which has never generated much interest by itself, so it makes sense to drop an npm dependency. moog-require too.

[boutell]

We can't use super as an argument name, it's a reserved word. We can write:

        extendMethods(self, options) {
          return {
            doubler(_super, times) {
              // now it's a quadrupler!
              return _super(times) * 2;
            }
          };
        }

[ammein]

Hmm, indeed super is reserved word. However, I was thinking about a proper naming arguments without using underscore at begining of super word. Maybe we could teach devs to use superMethod as an argument ,for instance superDoubler instead of using _super . I know this is unnessecary but it helps me on better wordings than just an implicit matches using _super . What do you think ? @boutell

[ammein]

It provides clear naming like previous apostrophe tutorial that using super method like superGetCreateSingleton

[boutell]

I hear you but I think one of the wins here is that it is already explicit which method we are extending so we don't need the extra language.

…

On Sat, Jun 15, 2019, 3:50 AM Amin Shazrin ***@***.***> wrote: It provides clear naming like previous apostrophe tutorial that using super method like superGetCreateSingleton — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1940?email_source=notifications&email_token=AAAH27NOVIRFUKDXC4QAYP3P2SNLNA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXYSUUQ#issuecomment-502344274>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAH27MQ77YLEY2BZWIFQHTP2SNLNANCNFSM4HTK3NIA> .

[boutell]

No reason you can't do it though. It's just the first argument, whatever you choose to call it.

…

On Sat, Jun 15, 2019, 8:00 AM Tom Boutell ***@***.***> wrote: I hear you but I think one of the wins here is that it is already explicit which method we are extending so we don't need the extra language. On Sat, Jun 15, 2019, 3:50 AM Amin Shazrin ***@***.***> wrote: > It provides clear naming like previous apostrophe tutorial that using > super method like superGetCreateSingleton > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1940?email_source=notifications&email_token=AAAH27NOVIRFUKDXC4QAYP3P2SNLNA5CNFSM4HTK3NIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXYSUUQ#issuecomment-502344274>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAAH27MQ77YLEY2BZWIFQHTP2SNLNANCNFSM4HTK3NIA> > . >

[ammein]

Noted. I think it makes sense too and much more shorter. Besides , its in argument rather than declare it as variable which may cause duplication declare variable name.

[boutell]

Now that these will just be "apostrophe types", the "moogBundle" property can just become "bundle".

[boutell]

Per discussion today, revising the proposal with just a single, exported (self, options)... function containing everything else. Also adding a section for fields (with add, remove, arrange... subsections) that will eliminate pretty much all "options.addFields.concat..." boilerplate.

Moving to objects rather than arrays for schema configuration because it's clear and succinct to use the field's name as a key rather than a "name" property, because it expresses the requirement of uniqueness nicely, and because we can: ES6+ guarantees order when iterating properties.

[boutell]

I've just updated the proposal at the top of this ticket with two important changes:

1. There is just one wrapper function at the top level of the module to inject (self, options) into the scope. Makes the code much tidier. This is possible because we can invoke it twice, once just to get the inheritance-related properties (extend and improve), and another for the actual construction. There is no imperative code in a module's top level function (all it does is return a module object in the new format), so this shouldn't trouble anyone.

2. The new module format now addresses middleware as well.

There are smaller changes too; please have a look!

We are continuing with implementation of the 2.x module converter and the 3.x code to power these modules.

[boutell]

Today we sorted out that the single wrapper function is indeed not a good idea after all. The first invocation will have a bogus self and options, inviting bugs in code that reasonably assumes they exist and consults them to decide what to return. In addition, we looked at moving all the sections that do require self, options into a single scope function. However this too fails because it creates a situation where methods aren't attached to the object in time to be directly used as route functions. Although it is usually best practice to do input validation in an inline route function before calling methods, forbidding direct use of methods as routes is taking things too far.

So we're back to an older version of the proposal with more (self, options) functions.

However we also changed adjustOptions to beforeBaseClass, which is specific about exactly when it runs (they are invoked from bottom to top of the class hierarchy, unlike everything else). This feature is still a bit mysterious obviously but thanks to fields 90% of use cases for it have gone away in this proposal.

[boutell]

Added restApiRoutes section. While you can implement REST routes by using the empty string as the property name for GET and POST, this looks weird, and it's convenient to have a direct way of implementing the standard REST pattern.

[boutell]

beforeBaseClass renamed to beforeSuperClass, as "super" is the term of art in both apostrophe and ES6 classes (which we're not using, but there's no point in introducing a competing word).

[boutell]

I am actively implementing this in 3.x. Most of it is great, but after getting some experience with middleware and useMiddleware, I realized they were heavy and awkward. After further discussion with @stuartromanek and @abea we figured out that middleware should just replace the old expressMiddleware property and stay out of the business of delivering optional middleware to be used only in some routes, which is easy to do using methods. Then useMiddleware can go away. extendMiddleware is still available to ensure middleware behavior can be changed in subclasses.

[boutell]

Updated to add queryFeatures, extendQueryFeatures and a brief discussion of same.

[boutell]

Revised that to a queries section that takes (self, query) for context, and has builders and methods subkeys. builders is for what we used to pass to addFilters, while methods is for ordinary methods to be added to the cursor.

extendQueries would also be supported.

[boutell]

Today I spent a lot of time working out a thorny issue with the design of A3 modules.

It took a while, but I figured out that:

• We should call init() functions for the module's whole inheritance tree before we figure out the middleware, routes, apiRoutes, handlers, etc.
• This gives us access to decisions init() made when deciding what middleware we actually want, etc. Including basic stuff like self.apos being set.
• I thought we would need to introduce afterInit(), for those who want to invoke their own routes or events immediately after the module is initialized. However it turns out this doesn't make sense:

• For routes, it would never work before listen() anyway.
• For events, it would never reach modules configured after this module, and the whole point of events is to reach other modules; you might have your own listener, to keep your pattern consistent, but you would emit the event in an apostrophe:modulesReady handler, or even later.

I wrote epic notes as I worked through this thought process, but the end result is simple and you can see it here. It's better to write the right code, not more code.

Tomorrow I will implement this tweak and then conditional configuration of middleware will behave sensibly.

[boutell]

I have completed the refactoring to call init() when methods, fields and options are ready but before sections are built, so that the section functions can invoke methods to decide what routes they want to register, etc.

I did also wind up introducing afterAllSections(self, options), so that apostrophe-module (the base class of all modules) has an appropriate place to invoke methods that actually take self.routes and map over it and add routes to Express, for example. Project level and even the vast majority of npm module developers will never need to write an afterAllSections function.

[abea]

We talked about this a little bit, but I think it might be worth us thinking about documenting extenders with something like _original rather than _super as the extended function. super is a JS reserved word referring to something similar, however it's specific to Classes, so I don't know if we get the full value of using a familiar term.

[boutell]

Errata: * A bunch of cases where the opportunity to use method syntax was missed in the examples (still writing function). * extendMiddleware is probably just a bad idea because of the ambiguity of what `next` does. If you want your middleware to be extend-friendly it should be implemented by invoking a series of methods. * "beforeSave" is a poor example of a method for this guide because we are replacing that particular method with promise events. Can't remember if I said "oh yeah, that's a goof" about anything else on the call just now.

…

On Tue, Apr 14, 2020 at 2:34 PM Alex Bea ***@***.***> wrote: We talked about this a little bit, but I think it might be worth us thinking about documenting extenders with something like _original rather than _super as the extended function. super is a JS reserved word referring to something similar, however it's specific to Classes, so I don't know if we get the full value of using a familiar term. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1940 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH27JZFDI3ERXPAUTGY6LRMSUE5ANCNFSM4HTK3NIA> .

-- THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his