Server method definition dangers with the current recommended Meteor app structure

1

I know there’s already a variety of questions on these forums about file structure - but I have a directed question around reliably exposing server methods, and I don’t quite understand the intended workflow since the import-based structure that has been espoused since Meteor 1.3.

Consider the case of having some methods on the server - because you don’t really have to do anything with a method on the server to create the DDP endpoint, simply defining the method in a given file is typically all that’s done - export const myMethod = ... Now, since only explicitly imported files are included, you need to import './your-file' for every file which defines a method. Sucks, but okay.

Next month, you come along and add two methods - a and b - and the a method happens to call the b method (it’s perfectly valid to call other meteor methods from each other). Unfortunately, you only remember to explicitly import the file containing a in your server startup file - but things simply work since importing the file for a naturally imports b due to the dependency on it. This is no good, since now b will only be defined if referenced in a - but everyone will think it’s reliably there as its own method, like all the others.

Some time passes and someone else comes along and refactors a which now no longer depends on b. They test use cases for a thoroughly, everything works, merge and deploy. However, production breaks, claiming b is no longer a method - even though there were zero changes made to anything related to b.

Now, sure, you can argue the cases that you should have more tests, do better testing, etc. - but this seems a fundamental flaw in the recommended server-side setup that you need to remember to explicitly import every method export to ensure you don’t accidentally remove a method from the server side that’s used on the client.

In a traditional API design, you’d need to explicitly expose backend methods in some way such that the endpoints (REST, GraphQL, whatever) simply wouldn’t exist without being referenced from some more global object which exposes or in some way sends requests to them - but Meteor is special in that it creates endpoints simply by the fact of defining the method, no other configuration necessary.

Long story long, the explicit import recommendations on the server-side seem broken to me - I don’t see a reason I wouldn’t want every file in every server directory always included no matter what - there’s essentially no cost to server-side code size aside from some minor build/deployment time, but once it’s running, it doesn’t matter how much code there is - this isn’t the frontend where we’re passing big chunks of code over the wire. If you don’t want something to exist on the server, you should just delete the file anyway.

Has anyone else reached a reasonable solution to this aside from simply hoping you don’t screw up?

2

This normally happens to us almost every time we add a new method file i.e. we forget to import the file in the server during the first run.

I almost created a loader wherein it would require all files inside the methods folder in the server

3

Another approach would be to decouple your method code from your method bodies such that the methods are simple, thin apis whose sole purpose are to act as the communication endpoint between a client and your business codebase.

In such organization, your methods don’t call other methods, they are only callable from the clients, helping with this abstraction.

There are lots of benefits to this approach as simple as making your code more modular to allowing you to create methods, publications, rest enpoints, graphql resolvers (api endpoint codebase) around the same business codebase and either have them live side by side or migrate from one to another.

5 Likes
4

That’s also how I like to do it. Methods check arguments - which isn’t needed when the call comes from a server - and pass them along to regular functions.
You can even factor it a bit to end up with a singular method, but that’s another story.

1 Like
5

Thanks for the quick responses - and yeah, I considered the separation approach of pulling out business logic from the methods, but my situation is actually a little more complex that my example lets on - I utilize the transform: option for collections such that I wrap raw mongo documents with smarter model objects. So, say you have a Books collection, each item might be transformed such that the collection returns a more intelligent Book object for each book with a setTitle method - and that setTitle method then calls the meteor method. I could, of course, have setTitle do different things based on being called from client/server, but I’d rather avoid branched logic like that when it really shouldn’t be necessary.

I appreciate the value of abstractions, but right now there isn’t really any performance or other need to do it.

6

Good news, I figured out how to get the old behavior back - removing the entire “meteor” section from the package.json did the trick. No more worries, at least in the short term, of forgetting to import methods on the server-side

1 Like
7

Hmm, but does that not basically tell the server to eagerly load everything it can find in non-client and non-import folders, defeating the purpose of import based code organization?

8

It definitely does - but I’ll make a trade-off between a bigger server bundle for not having weird bugs show up.

Also, it’s worth considering why eager loading really matters on the server - there isn’t much point in having files on the server that you never use, and space is not at a premium on the server like it is on the client/over the network.

9

I have not been relying on that behavior for years now, but as far as I can remember, here are some potential gotchas:

  • you have some file in your server that you have not imported anywhere, perhaps you forgot to delete it. It will still load
  • if such loaded file overwrites or extends something else, you will have a hard time identifying the problem
  • any file that has a variable declaration without the var/const/let keyword will hoist that to global, potentially overwriting some already existing global, or simply pollute global and potentially expose private variables
  • your ide/editor will have a hard time helping you find that file within the code tree
  • your file load orders will have changed unexpectedly, based on meteor’s specific (old) file load order rules

The module system (import/export) is there for a reason, and dumping it for the original reason you’ve given sounds to me like a huge regression.

Yes, it is not really very ideal to have to import the method-containing-files explicitly and you seem to be relying heavily on rather unpopular meteor specific constructs like collection transform option in itself should not be a good excuse because there are better abstractions (such as https://atmospherejs.com/dburles/collection-helpers or the simple possibility to create your own custom collection class that extends Mongo.Collection) over that, that does not require you to sacrifice better code organization.

All in all, the bullet points I’ve listed above are very similar problems to what you have been facing with the forgotten imports, but as you see, while solving one problems, you are cornering yourself susceptible to a similar and arguably larger problems.

10

Thanks for your comments - I actually have started wrapping Mongo.Collection but for other reasons - I could also do transforms on that which is an interesting idea. I don’t think collection-helpers is quite the same as transforms since transforms allow you to create totally new objects where you can control data access - I have cases where a given model encapsulates functionality such that there’s no desire for the caller to have access to all fields, so I can keep the raw data model truly private from the client and the business layer (the models) is the manager of that.

Your points are valid, it’s just the trade-off I’m making right now. Knowing me I’ll probably completely change it all in a month, but for now, it’s working well. I’d argue the first 3 points are bugs that I’d be happy to have break things, and I haven’t run into issues on 4 or 5

2 Likes
11

I really don’t have any problem. Just add one import line in a startup file, I can code thousands of lines so just few more of them are really no big deal.
The most important part of doing it manually is I can easily disable/enable some methods that I don’t need but I don’t want to remove the file, for example some migration script.

12

That’s pretty much the gold standard.

You’ve pretty much hit the nail on the head for why tedious import management on the server never really made any sense. The more complex import behaviour, which despite being introduced all the way back in 2014 is still not correctly recognized by editors like Webstorm, was probably a big step backward from the “pile of files, depth-first-search” import system of Meteor 0.4-1.4.

Well that’s going to cause an incredible amount of grief. As a matter of using mongo the mongo way, consider always storing exactly what you need to render, and store more if you need to store some private business related data.

13

I don’t think there’s any need to take the recommendation as dogma and reject eager loading.
If eager loading works for you, use it!

I quite like the idea of only using eager loading on the sever like you’ve done

I personally don’t, because I prefer the explicit style. That choice was made 50% on aesthetics and 50% because when I took over this project, we had globals everywhere and I never knew where a particular function came from