- 
                Notifications
    
You must be signed in to change notification settings  - Fork 97
 
feat(dropdowns): migrate react-dropdowns to Typescript #371
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Can you comment on the Travis script reordering?
          
 Yes! This is due to typescript needing the type definitions in the nested packages  Moving the   | 
    
| exclude: /node_modules/u, | ||
| use: [ | ||
| { | ||
| loader: 'babel-loader', | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m curious @Austin94, why do you prefer to use babel-loader (with @babel/preset-typescript) instead of ts-loader (or ts-loader + babel-loader without @babel/preset-typescript)? What would you lose?
And a related question: have you tried to emit declaration files at the same time as when compiling TS?
Glad to see this PR by the way 😃.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you prefer to use babel-loader (with @babel/preset-typescript) instead of ts-loader (or ts-loader + babel-loader without @babel/preset-typescript)?
This matches the default webpack config that exists in create-react-app now.
have you tried to emit declaration files at the same time as when compiling TS?
This got me thinking about the webpack ordering and I've reconsidered the ts-loader vs @babel/preset-typscript decision.
I've now updated the build to use ts-loader -> babel-loader and emit those declarations during the build step. This should increase the build speed over time as we move more packages over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| test: /\.tsx?$/u, | ||
| loaders: [ | ||
| { | ||
| loader: 'eslint-loader' | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the separation here for tsx into two different loaders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loader uses enforce: 'pre' which I believe has to be enforced at a module rule level. We don't want to enforce that on ts-loader and babel-loader
ca1a51f    to
    0b538cf      
    Compare
  
    
Description
This PR is a redo of #369 😄
There have been some changes recently in the typescript parser that makes sticking with styleguidist more feasible than before.
For our components it's common for us to spread attributes to their semantic component. This is possible to type, but results in:
For automated prop generation this would show all 1000+ valid props, including our custom ones.
The
react-docgen-typescriptmodule has added some helpers for this where I can filter out any props coming fromnode_modulelibraries (like React in this case).Detail
Same as #369, but without Storybook.
The first commit is only the TS additions to the build and test infrastructure. The second is migrating dropdowns to TS.
The examples and component/testing logic have been left unchanged in this PR.
Checklist
designer as a reviewer)
component
yarn start)