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

ToDo: TypeScript #1

Closed
Templarian opened this issue Aug 15, 2018 · 11 comments
Closed

ToDo: TypeScript #1

Templarian opened this issue Aug 15, 2018 · 11 comments

Comments

@Templarian
Copy link
Owner

Templarian commented Aug 15, 2018

Rewrite this component in TypeScript 3 and include the definition files like we do for @mdi/js.

@azdanov
Copy link
Contributor

azdanov commented Aug 17, 2018

Are PR's welcome for this?

@Templarian
Copy link
Owner Author

@azdanov Usually I would say yes in a heartbeat, but I have it almost done. Definitely can look it over and make tweaks in a PR once I have it merged. 😄

@Templarian
Copy link
Owner Author

@azdanov https://github.com/Templarian/MaterialDesign-React/blob/master/src/Icon.tsx

Not even tested or compiled yet. Just committing a rough draft. It's really close to your PR. Busy tonight working on some other stuff, but will have more time this weekend.

@Templarian
Copy link
Owner Author

ToDo: Need to add the types definition file to the package.json.

@Templarian
Copy link
Owner Author

@azdanov Okay, so I've ran into a really dumb bug/issue. I can't get the propTypes to validate in a test project. VS Code clearly shows that they are required or not, but it doesn't throw an error in the browser in `mode=development'.

Basic example: <Icon /> Since path={} is required it should be throwing an error.

The propTypes are showing up in the compiled code. Tried to publish as dev for the Icon component it shows errors, but production removes errors. I assumed if the parent project was in dev that component would still report errors.

Kind of giving up for now and moving on to adding icons and will look into this shortly. Any help would be appreciated. Attached my sample app.

jsreact.zip
.

@azdanov
Copy link
Contributor

azdanov commented Aug 19, 2018

Add this to webpack.config.js:

externals: {
  react: "commonjs react",
  "prop-types": "commonjs prop-types"
}

@Templarian
Copy link
Owner Author

That worked. Awesome!

@azdanov
Copy link
Contributor

azdanov commented Aug 19, 2018

Probably same issue as React. Facebook still uses commonjs.

@azdanov
Copy link
Contributor

azdanov commented Aug 19, 2018

I'm wondering if webpack is needed. Since this is used in react, and probably with a build system (who still writes createElement?), then distributing es6 code is fine.

By itself tsc will transform typescript to javascript, and the rest is for the end-user to manage.

EDIT:

Forgot to mention some benefits:

  • Simpler build step
  • Bundle size is reduced (webpack bootstrap code)
  • Possibly enable tree-shaking if the library gets more features

@Templarian
Copy link
Owner Author

Yea, the @mdi/js only uses tsc. Considered simplifying this build also. We only need an ES6 module for this lib, so probably a good idea.

@Templarian
Copy link
Owner Author

Closing this and working on updating the build to use TSC with a new issue.

@azdanov Can you do a PR and add a contributors property to the package.json for yourself since you co-wrote this library.

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

No branches or pull requests

2 participants