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

Feature/load db #2

Closed
wants to merge 3 commits into from
Closed

Conversation

eSlider
Copy link

@eSlider eSlider commented Jun 12, 2020

No description provided.

@eSlider eSlider closed this Jun 12, 2020
@lovasoa
Copy link
Member

lovasoa commented Jun 12, 2020

@eSlider, this is a nice PR, why did you close it ?

Comment on lines +18 to +29
const me = this;
Promise.all([initSqlJs(), axios.get('./test.db', {responseType: 'arraybuffer'})]).then(res => {
const SQLite = res[0], dbStorage = res[1];
const db = new SQLite.Database(new Uint8Array(dbStorage.data));
// language=SQLite
// const rows = db.exec("SELECT count(*) FROM db_articles");
// console.log(rows);
me.setState({db: db});

}).catch(err => {
me.setState({err});
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const me = this;
Promise.all([initSqlJs(), axios.get('./test.db', {responseType: 'arraybuffer'})]).then(res => {
const SQLite = res[0], dbStorage = res[1];
const db = new SQLite.Database(new Uint8Array(dbStorage.data));
// language=SQLite
// const rows = db.exec("SELECT count(*) FROM db_articles");
// console.log(rows);
me.setState({db: db});
}).catch(err => {
me.setState({err});
});
Promise.all([
initSqlJs(),
fetch('test.db', {responseType: 'arraybuffer'})
]).then(([SQLite, dbStorage]) => {
this.setState({db: new SQLite.Database(new Uint8Array(dbStorage.data))});
}).catch(err => {
this.setState({err});
});

Copy link
Author

Choose a reason for hiding this comment

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

@lovasoa, thank you for the advise to use fetch. I will use it.

@@ -70,7 +81,7 @@ export default class App extends React.Component {
<textarea
onChange={e => this.exec(e.target.value)}
placeholder="Enter some SQL. No inpiration ? Try “select sqlite_version()”"
></textarea>
>SELECT count(*) FROM db_articles</textarea>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>SELECT count(*) FROM db_articles</textarea>
SELECT * FROM db_articles

@@ -1,8 +1,8 @@
import React from "react";
import axios from "axios";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import axios from "axios";

Comment on lines -12 to +13
<link rel="manifest" href="%PUBLIC_URL%/manifest.json">
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<!-- <link rel="manifest" href="%PUBLIC_URL%/manifest.json">-->
<!-- <link rel="shortcut icon" href="%PUBLIC_URL%/fav/favicon.ico">-->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<link rel="manifest" href="%PUBLIC_URL%/manifest.json">
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<!-- <link rel="manifest" href="%PUBLIC_URL%/manifest.json">-->
<!-- <link rel="shortcut icon" href="%PUBLIC_URL%/fav/favicon.ico">-->
<link rel="manifest" href="%PUBLIC_URL%/manifest.json">
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">

Copy link
Author

Choose a reason for hiding this comment

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

But where is the manifest?

Copy link
Member

Choose a reason for hiding this comment

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

Ooops 😁 You are right, there is no manifest, let's remove this line.

@@ -9,6 +9,7 @@
"main": "src/index.js",
"license": "MIT",
"dependencies": {
"axios": "^0.19.2",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"axios": "^0.19.2",

Copy link
Author

Choose a reason for hiding this comment

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

@eSlider, this is a nice PR, why did you close it ?

@lovasoa, it's breaks previous behavior.

Here are the changes:

  • Download an sqlite database file (axios)
  • Prepare downloaded file to handle it right (that was not easy :)
  • Add favicon to prevent 404 errors
  • Remove load of manifest.json - that not exists
  • Lock packages to get working version
  • Add an select SQL query as a value for textearea
  • Publish to another place: link
  • Circle CI:
    • Automated static content build
    • Publish build in a gh-pages branch
    • Disable jekyll post processing
    • Prepare demo to work over github pages CDN
    • Get load generated assets using relative paths (That what I am working on)

If you ok with, I will open PR again.

P.S. And thank you so much for you simple react-boilerplate! It's amazing!

Copy link
Member

Choose a reason for hiding this comment

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

Hello !

Download an sqlite database file (axios)

You don't need to add a dependency just to make an HTTP request. Let's just use fetch

Prepare downloaded file to handle it right (that was not easy :)

?

Add favicon to prevent 404 errors

✔️

Lock packages to get working version

✔️

Add an select SQL query as a value for textearea

✔️

Publish to another place: link
Circle CI:
Automated static content build
Publish build in a gh-pages branch
Disable jekyll post processing
Prepare demo to work over github pages CDN
Get load generated assets using relative paths (That what I am working on)

Let's just keep the current netlify CD (https://sqljs-react-demo.netlify.com/). I don't think we need to switch to github pages.

@lovasoa
Copy link
Member

lovasoa commented Jun 12, 2020

I highlighted a few suggested changes

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.

2 participants