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

Add a resolveModuleId and resolveModuleImport callback to options. #80

Merged
merged 2 commits into from
Jan 17, 2017

Conversation

ritzingerp
Copy link
Contributor

With this callback callers can provide a custom logic how the identifiers of referenced modules will be written into the generated d.ts

@dylans
Copy link
Contributor

dylans commented Jan 12, 2017

Thanks @ritzingerp . I think this looks fine. Will let it sit a little longer in case anyone else wants to review. Will land it for 2.1.0.

@dylans dylans added this to the 2.1.0 milestone Jan 12, 2017
@dylans
Copy link
Contributor

dylans commented Jan 15, 2017

@ritzingerp resolving #90 has introduced a small conflict here... please rebase and then I'll land this. Also, if you have time, please also add a PR to update the README. Thanks!

ritzingerp added a commit to ritzingerp/dts-generator that referenced this pull request Jan 16, 2017
…tePen#80)

With these callbacks callers can provide custom logic how the identifiers of declared and imported modules will be written into the generated d.ts
@ritzingerp ritzingerp changed the title Add a resolveModuleRef callback to options. Add a resolveModuleId and resolveModuleImport callback to options. Jan 16, 2017
@ritzingerp
Copy link
Contributor Author

The rebase is done. I have extended this pull request to not only be able to customize module ids in import, but also in declare module statements.

I have also created a unit test case.

Copy link
Contributor

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Other than a couple of minor indentation nits, this looks ready. Thanks for the effort @ritzingerp !

@@ -56,11 +74,11 @@ function getError(diagnostics: ts.Diagnostic[]) {
// the tsconfig.json don't; the messageText is enough to diagnose in those
// cases.
if (diagnostic.file) {
const position = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start);
const position = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the indentation here is off by one?

message +=
`\n${diagnostic.file.fileName}(${position.line + 1},${position.character + 1}): ` +
`error TS${diagnostic.code}: ${diagnostic.messageText}`;
message +=
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation change unintentional?

…tePen#80)

With these callbacks callers can provide custom logic how the identifiers of declared and imported modules will be written into the generated d.ts
@ritzingerp
Copy link
Contributor Author

I have fixed the indentation, it was not intended.

@ritzingerp
Copy link
Contributor Author

Originally this pull request introduced a breaking change.

For input file some/module.ts:

import { x } from 'x';

the following was generated before the pull request:

declare module 'name/some/module' {
    import { x } from 'x';
}

and originally with the pull request:

declare module 'name/some/module' {
    import { x } from 'name/x'; // 'name/' prepended to import, only if there was no declare module 'x' in input
}

The latest commit for this pull request reverses this behavior, and does not introduce a breaking change.

This might be related to #94, but I think this pull request should not include a breaking change.

Copy link
Contributor

@dylans dylans left a comment

Choose a reason for hiding this comment

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

One small change requested (Rel -> Relative).

@@ -0,0 +1,5 @@
export class NonRel {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be picky here and say that we should call this NonRelative throughout, given that Rel can have multiple meanings.

@ritzingerp
Copy link
Contributor Author

Agree, renamed to NonRelative.

@dylans
Copy link
Contributor

dylans commented Jan 17, 2017

Thanks @ritzingerp !

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