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

Change example to ES6 imports #400

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 9, 2021

  • Changes example project to use import / export instead of require / module.exports
  • Bumps dependancies

@dblythy
Copy link
Member Author

dblythy commented Sep 23, 2021

This would be the first branch of #401. I'm thinking this would be what the other branches are based off.

Includes a better example page that actually uses the JS SDK, closing #396.

Moves code to src as per #397.

@sadortun @mtrezza feel free to let me know your thoughts 😃

Copy link

@sadortun sadortun left a 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.

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

Copy link
Member Author

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 = {

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

Copy link
Member Author

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

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;

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.

Copy link
Member Author

@dblythy dblythy Sep 23, 2021

Choose a reason for hiding this comment

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

Hmmm 🤔

updateStatus('Please correct the invalid fields.');
return;
}
await resolve(Parse.User.logIn(username, password));

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

Copy link
Member Author

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();

Choose a reason for hiding this comment

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

Missing await?

Copy link
Member Author

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/parse@3.3.0/dist/parse.min.js"

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

Copy link
Member Author

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

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.

None yet

2 participants