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

Refactor FilesAdapter to ES6 style. #315

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Conversation

nlutsenko
Copy link
Contributor

Class-based, nice import style and friends, also removed duplicate logic.
cc @drew-gross

@peterdotjs
Copy link
Contributor

@nlutsenko we can also start replacing var with let and const as we're refactoring to es6.

@gnz00
Copy link

gnz00 commented Feb 9, 2016

I refactored the whole repo here: https://github.com/maysale01/parse-server/tree/es6-all-tests-passing

@gnz00
Copy link

gnz00 commented Feb 9, 2016

Can re-use some of that, it's been like ~60 commits. All tests were passing as of HEAD-2 commits or so, I started to make everything async and abstracted the cache and have 1 or 2 tests not passing.

@nlutsenko
Copy link
Contributor Author

Yeah, good call. The biggest point here is to create classes for pluggable modules - like Files or Database, but agree that we should probably do let/const instead of var.

@nlutsenko
Copy link
Contributor Author

Updated with let/const, slightly cleaner export/import syntax for GridStoreAdapter.

@facebook-github-bot
Copy link

@nlutsenko updated the pull request.

@peterdotjs
Copy link
Contributor

@maysale01 We definitely appreciate you making the es6 fork. The current plan is to es6ify files as we touch them moving forward.

@gnz00
Copy link

gnz00 commented Feb 9, 2016

@peterdotjs Yeah it's too far gone at this point anyways. Might be a good reference point, or just to copy some boilerplate. It's 1-1 for the most part.

@drew-gross
Copy link
Contributor

I don't really like this setAdapter function. Being able to change adapters while the server is running seems like a recipe for disaster. What happens to in-progress async actions? Can remove the setAdapter function?

@nlutsenko
Copy link
Contributor Author

@drew-gross What did you have in mind?
I can store the FilesAdapter on the server itself, but the previous implementation already had a global setAdapter, so ideas welcome here... Also might be a part of a separate PR.

@peterdotjs
Copy link
Contributor

@drew-gross since we're only using setAdapter during initialization would it be worth policing for incorrect usage? We could add a lock to prevent changes after ParseServer is initialized.

@drew-gross
Copy link
Contributor

I would prefer the configuration accept 3 different options: 1) a instance of an object that meets the FilesAdapter interface 2) the name of an npm module that will be required along with an options object that will be used to initialize (this is to allow pure json config) or 3) a class and the same options object.

Rather than having the function exist and trying to police the PRs to make it never gets called inappropriately, we should just remove it and guarantee it's never called inappropriately :p

See more discussion in #275 and #290

@flovilmart
Copy link
Contributor

@drew-gross I second that, and that could be extended for all adapters as discussed around #290

@gnz00
Copy link

gnz00 commented Feb 9, 2016

This is implemented per our discussion in #291. Needs to be reviewed

nlutsenko added a commit that referenced this pull request Feb 9, 2016
@nlutsenko nlutsenko merged commit 67959e5 into master Feb 9, 2016
@nlutsenko nlutsenko deleted the nlutsenko.es6.adapters branch February 9, 2016 20:41
@flovilmart
Copy link
Contributor

@maysale01 i can review now

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.

7 participants