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: add type definitions #88

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

harish-sethuraman
Copy link

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Tried to add type definitions to the loader using jsdocs

Breaking Changes

Adds type defs

Additional Info

Need inputs on how to move forward with this.

package.json Outdated Show resolved Hide resolved
callback(null, result.code, result.map.toJSON());

return;
}

// @ts-ignore sourceMap in the webpack doesn't have the type RawSourceMap
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid ignore

Copy link
Author

@harish-sethuraman harish-sethuraman May 11, 2022

Choose a reason for hiding this comment

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

Seems like the callback(null, ${content}\n${FOOTER}${exportsCode}, sourceMap); takes type def SourceMap that is not exported from webpack. Should we export that and consume it here?

Copy link
Member

Choose a reason for hiding this comment

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

Weird, it is interface with the same methods...

Copy link
Author

Choose a reason for hiding this comment

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

This is in webpack

declare interface SourceMap {
	version: number;
	sources: string[];
	mappings: string;
	file?: string;
	sourceRoot?: string;
	sourcesContent?: string[];
	names?: string[];
}

This is in source-map

export interface RawSourceMap extends StartOfSourceMap {
    version: string;
    sources: string[];
    names: string[];
    sourcesContent?: string[];
    mappings: string;
}

Copy link
Member

Choose a reason for hiding this comment

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

We should investigate it, maybe add better types, because ignore is not good idea for types, looks like problem in version, we can improve it

Copy link
Author

Choose a reason for hiding this comment

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

which version are you mentioning? webpack? It is in its latest version. I hope the source-map is in latest version as well?

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 export the SourceMap from webpack and consume it? Is it not recommended

Copy link
Member

Choose a reason for hiding this comment

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

I think webpack have own types for source maps due to this we have problems with types

Copy link
Author

Choose a reason for hiding this comment

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

Yep

Copy link
Author

Choose a reason for hiding this comment

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

Bump

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.

2 participants