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

feat(typescript): setup base infra #511

Merged
merged 1 commit into from
Jun 21, 2018
Merged

feat(typescript): setup base infra #511

merged 1 commit into from
Jun 21, 2018

Conversation

dhruvdutt
Copy link
Member

@dhruvdutt dhruvdutt commented Jun 14, 2018

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?

If relevant, did you update the documentation?

Summary
Add initial TypeScript support for @webpack-cli/init package.
Use npm run build and npm run watch for dev.

Does this PR introduce a breaking change?

Other information
#245

@@ -16,12 +16,12 @@ const propTypes = require("@webpack-cli/utils/prop-types");
*
* @param {Object} transformObject - An Object with all transformations
* @param {Object} config - Configuration to transform
* @returns {Object} - An Object with the transformations to be run
* @returns {Array} - An array with the transformations to be run
Copy link
Member Author

Choose a reason for hiding this comment

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

A bug in our docs caught by TypeScript. 😅

@@ -33,37 +33,38 @@ function mapOptionsToTransform(config) {
* and writes the file
*/

module.exports = function runTransform(webpackProperties, action) {
export default function runTransform(webpackProperties, action: string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be also done like (webpackProperties: any, action: string): any. I prefer not to bloat code with any.

If we don't want to use any, then we can explicitly define interface/type of whole webpackProperties object.

Copy link
Member

Choose a reason for hiding this comment

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

use any here, it's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid the use of any. Maybe we can leave a TODO to come back later once we have the interface. any is evil, it makes the use of a type checker basically pointless.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@evenstensberg
Copy link
Member

Before you do anything else, could you create a new next branch and target your PR towards that? I think this is the point where we need to actively do that

@dhruvdutt dhruvdutt changed the base branch from master to next June 14, 2018 20:18
@dhruvdutt dhruvdutt force-pushed the typescript branch 2 times, most recently from e883304 to 38ff991 Compare June 14, 2018 20:21
@dhruvdutt
Copy link
Member Author

By "new next branch", did you mean changing the base branch to next? What should be the base branch?

Currently, I've pushed the code to typescript branch on the current repo.

@evenstensberg
Copy link
Member

master is base branch but next is develop branch

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Taking this review in stages. I don't know if we should have the ts configs at webpack-cli or within each package. To avoid dupe code maybe its better to have it at the top scope (webpack-cli)?

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Also create the root package.json compilation. The build step should be done in "prepare" and publish after compilation. It should not include the ts files for each package.

@@ -33,37 +33,38 @@ function mapOptionsToTransform(config) {
* and writes the file
*/

module.exports = function runTransform(webpackProperties, action) {
export default function runTransform(webpackProperties, action: string) {
Copy link
Member

Choose a reason for hiding this comment

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

use any here, it's fine.

@@ -0,0 +1,14 @@
{
"compilerOptions": {
// Targeted standard
Copy link
Member

Choose a reason for hiding this comment

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

uncomment, invalid styntax and unneeded

* packages included when running the init command
* @returns {Function} creator/npmPackagesExists - returns an installation of the package,
* followed up with a yeoman instance of that if there's packages. If not, it creates a defaultGenerator
*/

module.exports = function initializeInquirer(...args) {
const packages = args.slice(3);
export default function initializeInquirer(...args: string[]) {
Copy link
Contributor

@ematipico ematipico Jun 15, 2018

Choose a reason for hiding this comment

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

I personally started to avoid default exports. Named exports are way better on the long run.

EDIT: this is just my personal opinion, we can leave them here

@webpack-bot
Copy link

@dhruvdutt Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

.gitignore Outdated
@@ -33,3 +33,6 @@ lerna-debug.log

# Yarn lock file
yarn.lock

# TypeScript generated files
packages/init/*.js
Copy link
Member

Choose a reason for hiding this comment

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

Should be `/packages/**/*.js

@@ -33,37 +33,38 @@ function mapOptionsToTransform(config) {
* and writes the file
*/

module.exports = function runTransform(webpackProperties, action) {
export default function runTransform(webpackProperties, action: string) {
Copy link
Member

Choose a reason for hiding this comment

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

ok

@evenstensberg
Copy link
Member

You will need to do a fresh rebase at next branch. It doesn't seem like the next branch has been updated yet. What is the status on that?

@dhruvdutt dhruvdutt changed the title feat(typescript): setup infra feat(typescript): setup base infra Jun 17, 2018
@dhruvdutt dhruvdutt force-pushed the typescript branch 4 times, most recently from 2846cc1 to 72c90e5 Compare June 19, 2018 12:34
package.json Outdated
"lint:codeOnly": "eslint \"{bin}/**/!(__testfixtures__)/*.js\" \"{bin}/**.js\"",
"lint": "eslint \"./bin/*.js\" \"./test/**/*.js\" \"{packages}/**/!(node_modules)/*.js\" ",
Copy link
Member

Choose a reason for hiding this comment

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

add tslint command as well and hook them up to ci and infra

package.json Outdated
"jsdoc": "jsdoc -c jsdoc.json -r -d docs",
"appveyor:lint": "lerna bootstrap && npm run lint",
"appveyor:test": "npm run test",
"semantic-release": "semantic-release",
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed now. Bad rebase? SR is removed from master

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing this. I think next branch will need rebasing as well.

Copy link
Member

Choose a reason for hiding this comment

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

git pull origin master

tsconfig.json Outdated
"exclude": [
"node_modules/**",
"packages/*/node_modules/**",
"**/*.d.ts"
Copy link
Member

Choose a reason for hiding this comment

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

If skipLibCheck is true, it will ignore the sourcemaps

@evenstensberg
Copy link
Member

This branch has conflicts that you'll need to resolve before going further 👍

Copy link
Member Author

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

Looks ready for the final round of review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants