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

Fix module augmentation in @parcel/transformers-typescript-types #7315

Merged

Conversation

astegmaier
Copy link
Contributor

@astegmaier astegmaier commented Nov 16, 2021

↪️ Pull Request

Edit: This PR is ready for a review - it works to solve the issues below, and includes a unit test to cover this case.

I'd appreciate any feedback - I'm new to this part of parcel, and I want to make sure I don't break anything.

There are two issues that this PR tries to solve, both identified in #7306:

  1. The current implementation of @parcel/transformer-typescript-types (intentionally) flattens all module declarations, including declarations that are intended to augment an external modules. Then, it will tree-shake things that it finds in the module augmentation, which (most of the time) will result in them being removed. This PR solves the issue by avoiding flattening "nested" module declarations - e.g. module declarations that appear within other module declarations.
  2. There is a bug where if you have a module declaration in the middle of your source file, then exports declared after that declaration get (mistakenly) removed, because we think they are unused e.g.:
    import { Person } from "original";
    Person.prototype.greet = function() { return `Hello ${this.name}!` }
    // Exports declared before the augmentation declaration are present.
    export const anotherThing: string = "hello";
    
    declare module "original" {
      interface Person {
        greet(): string;
      }
    }
    // Everything after this point will be missing from the .d.ts file.
    export const somethingElse: string = "goodbye"; 
    This PR solves the issue by using a stack to keep track of what level of module declaration you are in, and making sure to associate each node with the correct module even as you visit deep nodes, and then jump back out to the original parent node.

🚨 Test instructions

See the "should work with module augmentation" test in packages/core/integration-tests/test/ts-types.js.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@height
Copy link

height bot commented Nov 16, 2021

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@astegmaier astegmaier marked this pull request as ready for review November 17, 2021 05:22
@astegmaier astegmaier changed the title [DRAFT]: fix module augmentation in @parcel/transformers-typescript-types Fix module augmentation in @parcel/transformers-typescript-types Nov 17, 2021
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thank you!!

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.

None yet

3 participants