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

Option for adding variables with PropTypes. #1

Closed
wants to merge 4 commits into from

Conversation

ruslanxdev
Copy link

@ruslanxdev ruslanxdev commented Oct 1, 2018

Hi!
We need this case for add our d.ts interfaces to JS as PropTypes. This is a temporary solution when moving our internal library from JS to TS. In the circumstances of using our library in both JS and TS projects.

  • addVariables (boolean) - Adding variables for props. Defaults to false.
module.exports = {
  plugins: [['babel-plugin-typescript-to-proptypes', { addVariables: true }]],
};
// Example.d.ts
// Before
import React from 'react';

export interface Props {
  name?: string;
}

// After
import React from 'react';
import PropTypes from 'prop-types';

export const Props = {
  name: PropTypes.string
};
// Example.js
import React from 'react';
import Props from './Example.d.ts';

export class Example extends React.Component {
  static propTypes = Props;

  render() {
    return <div />;
  }
}

@ruslanxdev
Copy link
Author

I will try resolve conflicts.

@milesj
Copy link
Owner

milesj commented Oct 1, 2018

I'm curious why you're using/importing .d.ts for this?

@ruslanxdev
Copy link
Author

ruslanxdev commented Oct 2, 2018

This is a temporary solution.
Now our library is written on JS. But it is using in TS projects. Therefore, we need d.ts for our users, and we need PropTypes for us. But we don't want to duplicate it.
When we full rewrite our library on TypeScript we don't need PropTypes. We will use only TS types.

@milesj
Copy link
Owner

milesj commented Oct 2, 2018

@ruslankhh Just trying to understand your process. So are you generating TS declarations and then compiling the .d.ts files with Babel?

@ruslanxdev
Copy link
Author

ruslanxdev commented Oct 2, 2018

Nope)

  1. We write TS declarations in .d.ts.
  2. We write component in JS.
  3. We require .d.ts in JS.
  4. We compiling .d.ts to PropTypes with Babel
    4.1. ...in runtime (with webpack and babel-loader).
    4.2. ...in build with replace require('*.d.ts') to code (with babel-plugin-transform-ts-import).
  5. We set the PropTypes to component in JS.

@milesj
Copy link
Owner

milesj commented Oct 2, 2018

Unrelated to this PR, I'd highly suggest not using .d.ts in your actual application, as it means something very specific in the TS world.

src/addToClass.ts Outdated Show resolved Hide resolved
src/addToFunctionOrVar.ts Outdated Show resolved Hide resolved
tests/index.test.ts Show resolved Hide resolved
README.md Outdated
@@ -208,3 +208,43 @@ class Example extends React.Component {
}
}
```

- `addVariables` (boolean) - Adding variables for props. Defaults to `false`.
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of addVariables, let's call it exportVariableDeclarations.

Copy link
Author

@ruslanxdev ruslanxdev Oct 3, 2018

Choose a reason for hiding this comment

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

@@ -49,9 +53,10 @@ export default function addToClass(node: t.ClassDeclaration, state: ConvertState
if (propTypes) {
propTypes.value = mergePropTypes(propTypes.value, propTypesList, state);
} else {
const isVariable = addVariables && typeNames.length === 1;
Copy link
Owner

Choose a reason for hiding this comment

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

I think instead of assuming that typeName === a variable, you should check for imports and validate that it's actually a variable.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Author

Choose a reason for hiding this comment

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

For this?

export default class ClassStandard extends React.Component<{ name: string }> {
  render() {
    return null;
  }
}

Like that?

const isVariable = (
  state.options.declarePropTypeVariables &&
  typeNames.length === 1 &&
  state.componentTypes[typeNames[0]]
);

Copy link
Author

@ruslanxdev ruslanxdev Oct 3, 2018

Choose a reason for hiding this comment

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

Or do you mean to check if such a variable exists?
But it's redundant. We create it in the same location where we save this type in state:

state.componentTypes[node.id.name] = extractTypeProperties(
node,
state.componentTypes,
);
if (state.options.declarePropTypeVariables) {
addVariable(path, node.id.name, state);
}
},

Copy link
Author

Choose a reason for hiding this comment

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

Or do you think I'm adding a variable just for export?
That's not so. A variable is created for all types:

interface Props {
name: string;
}
const Props = {
name: _pt.string.isRequired
};
export default class ClassCustomReactImportNamePure extends R.PureComponent<Props> {
static propTypes = Props;
render() {
return null;
}
}"

state.componentTypes[node.id.name] = extractTypeProperties(
node,
state.componentTypes,
);

if (state.options.declarePropTypeVariables) {
addVariable(path, node.id.name, state);
Copy link
Owner

Choose a reason for hiding this comment

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

We should only add variables when the interface is for props/state. We shouldn't do it for every interface.

Copy link
Author

Choose a reason for hiding this comment

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

How we can do it? Match with the name suffix Props|State?

Copy link
Owner

Choose a reason for hiding this comment

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

You'll need to use the extracted generic names from classes/functions, and cross reference that.

Copy link
Author

Choose a reason for hiding this comment

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

What if we don't have classes/functions in d.ts?)

Copy link
Author

Choose a reason for hiding this comment

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

Or we should write everytime classes/function in d.ts?

Copy link
Owner

Choose a reason for hiding this comment

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

I won't allow this feature if it generates for every interface/type alias that exists. That would blow up the filesize unnecessarily.

Maybe if the setting was a regex pattern:

declarePropTypesVariables: /(Props|State)$/,

Copy link
Author

@ruslanxdev ruslanxdev Oct 8, 2018

Choose a reason for hiding this comment

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

I will think about it.

src/index.ts Show resolved Hide resolved
@ruslanxdev
Copy link
Author

Our conversation got me thinking.
Maybe you're right and this is too specific for your plugin. I decided to make it all into another plugin that only works for definitions.
https://github.com/ruslankhh/babel-plugin-ts-definitions-to-proptypes
If you are interested in this PR, I can fix it as you suggest and you will merge it.

@milesj
Copy link
Owner

milesj commented Oct 9, 2018

I think a fork might be best, as it's a very specific use case. Glad you got it working.

@milesj milesj closed this Oct 9, 2018
@ruslanxdev ruslanxdev deleted the ruslankhh.add-variables branch October 10, 2018 15:49
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