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

[Typescript] 3 un-typed exports from @material-ui/core #19258

Closed
2 tasks done
JonKrone opened this issue Jan 15, 2020 · 6 comments · Fixed by #19337
Closed
2 tasks done

[Typescript] 3 un-typed exports from @material-ui/core #19258

JonKrone opened this issue Jan 15, 2020 · 6 comments · Fixed by #19337
Labels
core Infrastructure work going on behind the scenes good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@JonKrone
Copy link
Contributor

JonKrone commented Jan 15, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

@material-ui/core exports 3 references that are not available in the typescript definitions:

isString, isNumber, and styleFunction

Expected Behavior 🤔

@material-ui/core should either not export these variables or provide Typescript types for them.

Steps to Reproduce 🕹

See the testExports.ts file and the console output in this demo: https://codesandbox.io/s/optimistic-wilbur-98lq2

Context 🔦

We are trying to dynamically generate an index.ts file that re-exports all members of @material-ui/core, except those which we have defined in our wrapping library. e.g., we want to customize the function of the TextField elements, so want to override that element with our own export but just re-export most other Material components.

While generating this file, we realized that not all of the actually-exported members of @material-ui/core had types for them.

Your Environment 🌎

Tech Version
Material-UI v4.8.3
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 15, 2020

I don't think that any of the modules of styles/transitions.js, aside duration and easing, should be available in the barrel index. I would suggest:

diff --git a/packages/material-ui/src/styles/transitions.js b/packages/material-ui/src/styles/transitions.js
index 1cc6e1ad1..b4839a8a8 100644
--- a/packages/material-ui/src/styles/transitions.js
+++ b/packages/material-ui/src/styles/transitions.js
@@ -28,9 +28,9 @@ export const duration = {
   leavingScreen: 195,
 };

-export const formatMs = milliseconds => `${Math.round(milliseconds)}ms`;
-export const isString = value => typeof value === 'string';
-export const isNumber = value => !isNaN(parseFloat(value));
+function formatMs(milliseconds) {
+  return `${Math.round(milliseconds)}ms`;
+}

 /**
  * @param {string|Array} props
@@ -52,6 +52,9 @@ export default {
     } = options;

     if (process.env.NODE_ENV !== 'production') {
+      const isString = value => typeof value === 'string';
+      const isNumber = value => !isNaN(parseFloat(value));
+
       if (!isString(props) && !Array.isArray(props)) {
         console.error('Material-UI: argument "props" must be a string or Array.');
       }

I believe these methods were only exported recently with #18306.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 15, 2020

Regarding styleFunction, the intent was to export and documented it for usage with styled-components and emotion, but it seems I never completed this task. Maybe we can remove the export now, and defer the concern to, later on, I'm wondering?

@JonKrone
Copy link
Contributor Author

JonKrone commented Jan 15, 2020

Awesome, I appreciate the speedy response.

If that is the approach you'd like to take for styleFunction, I would be happy to make these changes (probably later tonight/tomorrow), or if you want to take the time to document it now, I'll leave it for you as it'd take some time for me to do that properly.

@oliviertassinari
Copy link
Member

For styleFunction, I don't know. I wish we can get more inputs on this matter.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jan 17, 2020
@oliviertassinari
Copy link
Member

@JonKrone Do you want to take care of the modules in styles/transitions.js :)?
(We can handle styleFunction later on)

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Jan 18, 2020
@JonKrone
Copy link
Contributor Author

JonKrone commented Jan 21, 2020

Sure thing - made the above PR :).

There's probably a way to add this to the test suite. Could probably use the underlying typescript parser, ask it to generate a list of the symbols exported by the src/index.d.ts declaration file, then compare that with the list of symbols exported by a full module import/require.

(Just as an aside, I've +1'd the Issue thread about transitioning @material-ui to Typescript)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants