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

Allow cloud code file to be an array of files or a folder #6962

Closed
wants to merge 2 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Oct 22, 2020

Sorry again for submitting so many PRs, I've had a few ideas of how to improve my Parse Server I thought I'd share with the community.

For my backend, I like to break down my functions by class name, and then import the files in main.js (e.g /cloud/functions contains "_User.js", which has all user beforeSave and afterSave functions, and any Parse.Cloud.define functions relevant to Parse.User).

This PR allows the cloud parameter to be a folder, where all .js files will be imported, or an array of files. This allows users to have more, smaller, organised cloud files, which in my experience, makes it easier to navigate.

@ghost
Copy link

ghost commented Oct 22, 2020

Danger run resulted in 1 fail and 1 markdown; to find out more, see the checks page.

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #6962 into master will increase coverage by 0.02%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6962      +/-   ##
==========================================
+ Coverage   93.78%   93.81%   +0.02%     
==========================================
  Files         169      169              
  Lines       12269    12281      +12     
==========================================
+ Hits        11507    11521      +14     
+ Misses        762      760       -2     
Impacted Files Coverage Δ
src/ParseServer.js 89.61% <86.66%> (+0.14%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.58% <0.00%> (+0.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c68d055...b193dad. Read the comment docs.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have few comments about the code.

In addition, it is kinda strange to me to have multiple entry points for cloud code. Most of the frameworks that I know have a single entry point, from which the developers decide what more they want to load. On the other hand, I don't see any problem in having multiple entry points, except that, for the case of the directory, we can't ensure a specific order in which the files will be loaded. @dplewis @mtrezza @Moumouls thoughts?

src/ParseServer.js Show resolved Hide resolved
src/ParseServer.js Outdated Show resolved Hide resolved
src/ParseServer.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2020

If I understand this PR correctly, the advantage is that instead of adding require for external files in main.js, parse server requires automatically when specifying a folder for cloud code.

  • What happens if there are multiple, identically named functions in different files / the same file? Does this PR still detect that?
  • How would this work in a scenario where each of the files would need to access a shared code base? E.g. You have files chat.js, user.js, but each needs to access a jsonParser.js file?
  • Should each individual file have its own namespace, or should that be resolved, and how to handle conflicts as part of resolving?

@Moumouls
Copy link
Member

Moumouls commented Oct 22, 2020

Here i see some issues i found. In full CLI usage i understand the idea.
But here it seems that we have a try to introduce a dynamic custom code importer inside parse server and the solution will not work with many compiler like Typescript.
I think here we are on the wrong way.

Currently the cloud property support a function to lazy load all functions. So in your case for better DX you just need to do something like this.

// _User.js

Parse.Coud.define(xxxxx)
// main.js
modules.exports = () => { require('./_User')}
//server.js
const cloudCode = require('./main')

ParseServer.start({
...params,
cloud: cloudCode
})

I'm currently using a similar solution with Typescript support and it work like a charm

If you want a more concrete example see my repo (in TS):
https://github.com/Moumouls/next-atomic-gql-server/blob/master/src/server.ts

Personally I'm not sure this addition is a good idea, we will have many issues in the futur since parse server is not an importer system (like webpack)

Finally, i think that here we have a lack of best practice on cloud code guide, the cloud code as function is not correctly documented 😄

In any case @dblythy , thank you anyway to propose PRs it is always a plus for the evolution of Parse.

@dblythy
Copy link
Member Author

dblythy commented Oct 22, 2020

If I understand this PR correctly, the advantage is that instead of adding require for external files in main.js, parse server requires automatically when specifying a folder for cloud code.

Correct. Even though require is pretty easy to use, breaking up the cloud code was one thing I personally struggled to get my head around when I started with Parse, as I'd had no JS / backend experience. A lot of my PRs are around highlighting things that trapped me earlier on that I could've done better, had've I known what I was doing 😂.

  • What happens if there are multiple, identically named functions in different files / the same file? Does this PR still detect that?

Yes, according to my testing it registers all Parse.Cloud functions as expected.

  • How would this work in a scenario where each of the files would need to access a shared code base? E.g. You have files chat.js, user.js, but each needs to access a jsonParser.js file?

You would still have to require jsonParser.js in each individual file I believe. If you'd like me to write a test case for that I can.

  • Should each individual file have its own namespace, or should that be resolved, and how to handle conflicts as part of resolving?

I didn't think about that. I was more giving focused on the Parse.Cloud functions, but if we revisit this in the future, we can look at that.

  • But here it seems that we have a try to introduce a dynamic custom code importer inside parse server and the solution will not work with many compiler like Typescript.

I'm unfamiliar with Typescript, so thanks for pointing that out!

Thank you for your feedback @mtrezza, @Moumouls, and @davimacedo! It seems that this will incur more problems than it'll solve. So i'll close this now. Thanks all!

@dblythy dblythy closed this Oct 22, 2020
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.

4 participants