Types for packages

[rishabh3112]

refers #577

[ematipico]

Do they actually exist those files? The index.d.ts

[rishabh3112]

They don't that's why this PR is for @ematipico

[ematipico]

What I am saying is that these files they don't exist inside the various packages (migrate folder, for example). So, referring to them inside the package.json is not enough, it won't fix the issue; we have to create them. If you did, I don't see them in this PR

[rishabh3112]

Have set 'declaration': true in tsconfig.json
when building it will create a declaration file named <filename>.d.ts. and index.ts was the file whose type declaration has to be there in the package so that's why setting types to index.d.ts.

[rishabh3112]

If the whole codebase has been shifted to typescript
then can I remove 'allowJS': true from tsconfig.json
@ematipico

[ematipico]

Unfortunately we still need it I think, probably for the tests? I think @dhruvdutt can answer your question in a better way

[dhruvdutt]

Some parts of the codebase like the entrypoint folder is still in JS but this shouldn't affect the TS compilation process. We can remove probably allowJs after verifying the sanity. We have kept it because the TS migration happened incrementally.

About the index.d.ts file, I don't think it makes sense for all packages. IMO it only makes sense for webpack-scaffold. Maybe webpack-utils as well. We'll need to invest and verify the sanity of auto-generated interfaces as well. I'll probably take a look into this. Thanks for pointing it out. 🤗

Also, just a friendly note to not override the PR template. You don't need to fill it completely, you can ignore the questions/fields which aren't relevant to your PR but please keep the template consistent. 🙂

[evenstensberg]

About the index.d.ts file, I don't think it makes sense for all packages. IMO it only makes sense for webpack-scaffold.

Why?

[evenstensberg]

Just a small comment on commit format. type(scope): msg. In order for tests to pass. Important to follow convention so that the code doesn't get messy

[rishabh3112]

Members and Contributors,
Can I work on this issue for now?
(asking as @dhruvdutt self assigned it earlier)

[dhruvdutt]

@ev1stensberg: index.d.ts file makes sense only for webpack-scaffold because that's the only package we expose outside / people can install, import and consume this packages inside their applications/packages.
Rest of all packages are getting used internally inside TypeScript itself. No need to create interfaces for that unless we intend to expose them outside for users to import and consume. Do we?

@rishabh3112: You can definitely work on this. I think it needs some inputs from my end, but I'm always open to review/suggest. :)

[rishabh3112]

Okay, @dhruvdutt.
So should I rebase and export declarations it for webpack-scaffold only?

[webpack-bot]

@rishabh3112 Thanks for your update.

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

@ematipico Please review the new changes.

[evenstensberg]

What's the reason for declarations suffix?

[rishabh3112]

As it is used when declaration of ts files is needed (i.e. ts files in packages) @ev1stensberg

[evenstensberg]

Could it be done without the suffix?

[rishabh3112]

Yes.
What name do you want then?

[ematipico]

Maybe tsconfig.packages.json? What do you guys think?

[rishabh3112]

Why this PR was closed @ematipico?

[ematipico]

Apologies! I might have hit the wrong button

[rishabh3112]

@ematipico then should I proceed with your suggestion?

[ematipico]

Could it be done without the suffix?

@ev1stensberg we need to have a specific configuration only for those packages that need to expose the interfaces, so we need to keep separated configurations.

About the naming, yes let's call it with packages suffix as I suggested, it helps to understand what's the configuration for. Other than that, I am really happy with this PR!

[evenstensberg]

Oki :)

[webpack-bot]

@rishabh3112 The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (failure) and fix these issues.

[rishabh3112]

Do I need to rebase here for commitlint to pass or you may reference the latest commit @ematipico?

[dhruvdutt]

@rishabh3112 No need to stress about the commitlint. We'll rewrite the message during merge. We got it covered.

Can you please fix the conflicts?

[ematipico]

Can you please fix the conflicts?

PR is good to go :) no conflicts

[rishabh3112]

As far as i can see it says branch has no conflicts with base branch @dhruvdutt

[dhruvdutt]

🤔