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

test: "export const" in .ts module #36

Merged
merged 7 commits into from
Jun 15, 2019
Merged

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Jun 15, 2019

This test fails with the following output:

- /// <reference types="react" />
- declare type JSXElements = keyof JSX.IntrinsicElements;
- declare const elements: JSXElements[];
+ export declare type JSXElements  JSX.IntrinsicElements;
+ export declare const elements: Jnts[];
  export { JSXElements, elements };

@aleclarson aleclarson force-pushed the patch-3 branch 2 times, most recently from 111302c to 5c987c5 Compare June 15, 2019 01:08
@aleclarson
Copy link
Contributor Author

Looks like two bugs with one stone!

@aleclarson
Copy link
Contributor Author

Hmm, looks like we'll need to prepend the removed comment pragmas post hoc.

Preserving certain comments would be a non-trivial change, especially since by default, acorn does not help us at all in associating comments with Nodes so everything needs to be done by hand.

@aleclarson
Copy link
Contributor Author

Alright, the test is passing now! 🎉

With ca5ba31, any reference directives are collected in the transformFile function, and then they're re-added during the renderChunk step.

Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

Some coding style suggestions.
Plus, I would very much prefer to use the properties which are defined in the TS API instead of manually typing things.
I will most likely only look at those things tomorrow on my work machine.

tests/testcases/export-ts/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
@@ -56,6 +46,35 @@ const plugin: PluginImpl<{}> = () => {
return { source, program };
}

// Parse a TypeScript module into an ESTree program.
const references = new Set<string>();
Copy link
Owner

Choose a reason for hiding this comment

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

I don’t really have a better idea right now, just keep in mind that introducing more global state makes it harder to fix watch mode (#35)

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated

// Collect any reference directives
const pragmas: PragmaMap = (input as any).pragmas;
if (pragmas && pragmas.has("reference")) {
Copy link
Owner

Choose a reason for hiding this comment

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

since you check this here, is your type declaration really correct? (nullability?)
Also, your manual type declaration plus the as any cast suggests that this is not officially documented TS API?

A quick check with https://astexplorer.net/#/gist/577600e28bd89938dc16f5bf445980d4/0213bb7d7515552ab575dabaec61561145080b5d showed me that the sourceFile has a typeReferenceDirectives array. Maybe using that would be a better idea?
(these is also libReferenceDirectives)

That way, you can really re-create the tripleslash references manually.
Regarding the issue you opened for rollup, I think it is guaranteed to remove those comments, right? So we will not end up with a case where we end up with duplicated comments, right?

Copy link
Contributor Author

@aleclarson aleclarson Jun 15, 2019

Choose a reason for hiding this comment

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

Nice, I didn't see the typeReferenceDirectives property before.

I think it is guaranteed to remove those comments, right? So we will not end up with a case where we end up with duplicated comments, right?

I don't think so. I see a preserved directive in the .d.ts bundle of @react-spring/web. 😕 Looks like we'll need a test case for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, that might be because I'm using the renderChunk hook to prepend the comment pragmas. There might be a better place to do that?

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like we'll need a test case for that.

yes, that was also my thought to test that comments that are attached to items are correctly preserved / removed. that pragma is kind of weird, according to astexplorer its attached to the export statement, but that does not really make sense IMO.

Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

nice!

src/index.ts Outdated Show resolved Hide resolved
@Swatinem
Copy link
Owner

Thank you very much :-)
I will publish this tomorrow, and possibly play a bit more with comment attachment when I find some time.

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