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

Included Angular example #40

Closed
wants to merge 1 commit into from
Closed

Included Angular example #40

wants to merge 1 commit into from

Conversation

nelson777
Copy link

Working Angular example from this Stackoverflow question

@buzz
Copy link
Owner

buzz commented Jul 24, 2020

Thank you for the contribution.

I'm not quite sure if providing a full-fledged Angular example is really necessary. Maybe just linking to the SO question in README.md is more appropriate? In the end it's just one line in angular.json that does the trick. A topic that is described in depths in the excellent Angular documentation.

Apart from that:

WASM module

Do not commit the WASM module to git.

It should read

"assets": [
  "node_modules/mediainfo.js/dist/MediaInfoModule.wasm"
],

Better even use ../../dist/MediaInfoModule.wasm and a local path dependency in your package.json:

    "mediainfo.js": "file:../../",

That way I wouldn't need to keep the example up-to-date manually with every release.

Keep it simple

Keep the example to a bare minimum. It seems the example code has been sourced from some boilerplate. It includes numerous dependencies and features which create unnecessary complexity. Simplicity should be highest priority when creating code examples. So, strip any unnecessary cruft from the example package or start from an empty NPM package.

Explicitly name the crucial steps

What about the browser field in package.json. Is this necessary? If yes, it should also be explicitly mentioned in README.md otherwise removed from package.json.

@nelson777
Copy link
Author

nelson777 commented Jul 24, 2020

Thanks for your comments. I'll do the changes you proposed and get back to you.

I have already removed everything that I thought it wasn't needed, as tests and other things. But I'll take a look if I can delete anything else.

Browser section is needed or the project's build will fail. I'll change readme.

@nelson777 nelson777 closed this Jul 24, 2020
@nelson777
Copy link
Author

I'm creating another PR with proposed 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.

None yet

2 participants