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

CommentCompilationWarning typings #7532

Merged
merged 4 commits into from
Jun 23, 2018
Merged

CommentCompilationWarning typings #7532

merged 4 commits into from
Jun 23, 2018

Conversation

Legends
Copy link
Member

@Legends Legends commented Jun 14, 2018

What kind of change does this PR introduce?
CommentCompilationWarning typings

Did you add tests for your changes?
no

Does this PR introduce a breaking change?
no

What needs to be documented once your changes are merged?
no

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

* @property {number} line
*/

/** @typedef {Object} Loc
Copy link
Member Author

@Legends Legends Jun 14, 2018

Choose a reason for hiding this comment

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

Should I add @property {number=} index to @typedef {Object} Loc like in compareLocations.js ==> @typedef {Object} NodeLocation ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mohsen1 The Loc type I have defined here is used in several locations, is there a way to not typedef it over and over again, but just import it from a common definition file?

Copy link
Member

Choose a reason for hiding this comment

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

You can import typedefs from other files. Maybe the best place is in ``Dependency.js`

Copy link
Member Author

Choose a reason for hiding this comment

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

@ooflorent Can you make a comment on sokras comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Importing types from Dependency class makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please review the changes i made, advise appreciated. I have commited this change, but I have TS errors

image

Actually, I have no clue how to do this ;-)

* @property {number} line
*/

/** @typedef {Object} Loc
Copy link
Member

Choose a reason for hiding this comment

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

You can import typedefs from other files. Maybe the best place is in ``Dependency.js`

@webpack-bot
Copy link
Contributor

@Legends Thanks for your update.

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

@sokra Please review the new changes.

* @property {Position} end
*/

exports Position;
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to export types

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove the exports.. and leave everything as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sokra done.

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@@ -6,7 +6,19 @@

const WebpackError = require("./WebpackError");

/** @typedef {import("./Module.js")} Module */

/** @typedef {import("./Dependency.js").Position} Position */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused type

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

See @mohsen1 comment

@sokra sokra merged commit 2bc5437 into webpack:master Jun 23, 2018
@sokra
Copy link
Member

sokra commented Jun 23, 2018

Thanks

@Legends Legends deleted the CommentCompilationWarning branch June 23, 2018 11:17
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

5 participants