-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Change example to ES6 imports #400
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This will really help!
I left a few suggestions, feel free to ignore them if you want 😅
* `/src/public` contains public assets. | ||
* `/src/views` contains views that express can render. | ||
* `/src/config.js` contains all Parse Server settings. | ||
* `index.js` is the main entry point for `npm start`, and includes express routing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename index
to server.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s pretty common practise for index to be the main entry point for package.json
if (!databaseUri) { | ||
console.log('DATABASE_URI not specified, falling back to localhost.'); | ||
} | ||
export const config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a type defined for options I think ? ParseServerOptopns
?
You should cast it as that type so user have code completion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require babel/typescript though right? I was thinking we could leave that to a TS branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, if you import ParseServer, you should also be able to import the Options type
@@ -0,0 +1,14 @@ | |||
const databaseUri = process.env.DATABASE_URI || process.env.MONGODB_URI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help with visual clarity, here's 2 ideas
-
Consider add a helper function that either return the ENV if defined, or default string.
-
Collect/prepare all variables before the big
config
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm 🤔
src/public/assets/js/script.js
Outdated
updateStatus('Please correct the invalid fields.'); | ||
return; | ||
} | ||
await resolve(Parse.User.logIn(username, password)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you await resolve()
and not just await Parse.User.login()
?
Maybe consider renaming resolve
to updateStatus
or similar. resolve
is associated to the thing you call when you completed work inside a promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it’s a utility function that handles UI work and resolves the promise. Maybe I should cut it out from this simple version
async function saveObject() { | ||
if (testObjectSaved) { | ||
showTab(2); | ||
findObjects(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing await
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! This is for moving to the next step after user has already saved an object
</div> | ||
<script src="/public/assets/js/script.js"></script> | ||
<script | ||
src="https://npmcdn.com/[email protected]/dist/parse.min.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably for another day ;) but we really need to be able to import Parse from 'parse'
- It would allow us to auto update it with NPM
- Do tree shaking to reduce package size
- better autocompletiom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can browsers use node imports? Wouldn’t we need a compiler
import / export
instead ofrequire / module.exports