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

Added create react app example with typescript and basic usage #11220

Merged
merged 12 commits into from
Oct 10, 2019

Conversation

luistak
Copy link
Contributor

@luistak luistak commented Oct 9, 2019

Following this issues:

#8204 (comment)

and

#8305 (comment)

i have added a simple create react app using pdfjs

@luistak luistak changed the title Added create react app with typescript and basic usage Added create react app example with typescript and basic usage Oct 9, 2019
@Snuffleupagus
Copy link
Collaborator

i have added a simple create react app using pdfjs

Adding close to 20000 lines of code doesn't really seem like a "simple" example to me, is all of this actually necessary!?

One thing that needs to be seriously considered here is to not commit the yarn.lock files (especially the top-level one since we're using package-lock.json in this library), since this is only an example and not shipping code.

@luistak
Copy link
Contributor Author

luistak commented Oct 9, 2019

i have added a simple create react app using pdfjs

Adding close to 20000 lines of code doesn't really seem like a "simple" example to me, is all of this actually necessary!?

One thing that needs to be seriously considered here is to not commit the yarn.lock files (especially the top-level one since we're using package-lock.json in this library), since this is only an example and not shipping code.

I apologize for committing the yarn lock files in this issue, already fixed it.

I referenced the PR as a simple react app, because i just run npx create-react-app my-app and added a simple usage of PDFjs with an app created with create-react-app

Copy link
Collaborator

@Snuffleupagus Snuffleupagus 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 removing the yarn.lock files!
Please also see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits


Are the *.png files completely necessary here?

package.json Outdated
@@ -15,6 +15,7 @@
"escodegen": "^1.12.0",
"eslint": "^6.4.0",
"eslint-config-prettier": "^6.3.0",
"eslint-config-react-app": "^5.0.2",
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Oct 9, 2019

Choose a reason for hiding this comment

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

Is it necessary to add this "globally", or would it suffice to add it to the example-specific package.json file?
(If so, the package-lock.json file can be reverted as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was needed before i add typescript to the project, after that it's really not necessary, already removed it

Copy link
Contributor Author

@luistak luistak Oct 9, 2019

Choose a reason for hiding this comment

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

I remembred the reason of the config, it's because of the Ci failure, should i add it again?

image
Link

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it work to add it to examples/create-react-app/package.json file, rather than the top-level package.json file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't work, i tried it, only in the top-level :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't think we should have this here because the example should be self-contained. If need be, we can exclude the directory from ESLint, but I would suggest trying to remove the following lines from your package.json first:

  "eslintConfig": {
    "extends": "react-app"
  },

Perhaps that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll try it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worked! Thanks @timvandermeij!

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

In general this looks good and I'm glad that useful examples are being contributed!

examples/create-react-app/public/manifest.json Outdated Show resolved Hide resolved
examples/create-react-app/src/app.tsx Outdated Show resolved Hide resolved
examples/create-react-app/src/index.tsx Outdated Show resolved Hide resolved
examples/create-react-app/src/pdf.tsx Outdated Show resolved Hide resolved
package.json Outdated
@@ -15,6 +15,7 @@
"escodegen": "^1.12.0",
"eslint": "^6.4.0",
"eslint-config-prettier": "^6.3.0",
"eslint-config-react-app": "^5.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't think we should have this here because the example should be self-contained. If need be, we can exclude the directory from ESLint, but I would suggest trying to remove the following lines from your package.json first:

  "eslintConfig": {
    "extends": "react-app"
  },

Perhaps that works?

@timvandermeij timvandermeij merged commit 00c3339 into mozilla:master Oct 10, 2019
@timvandermeij
Copy link
Contributor

Thank you for providing this example and making it self-contained!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants