Skip to content

[Code Review] Additional concerns about recent namespace handling changes (not released) #359

@typhonrt

Description

@typhonrt

Greets @privatenumber,

I really appreciate your contributions to rollup-plugin-dts.

I have continued to review your most recent changes and there seems to be one additional problem and one "annoyance" with PR #352. One of them is a definite problem that needs to potentially be addressed and another is an "annoyance" or likely unnecessary change in output types that likely can also be omitted. I will first discuss the problematic code change which removes the export keyword from nested namespaces followed by the "annoyance" which is likely an unnecessary invocation of duplicateExports on the nested namespace.

I am testing with the latest unpublished rollup-plugin-dts main repo as of (10/15/25) that has your changes incorporated. I am using NPM link to my complex project w/ nested namespaces / types.


In particular lines 596-605. This block of code removes the export keyword from nested namespaces. This effectively alters public scope availability of the nested namespaces making the nested namespace private. This affects downstream consumption for both types usage and API documentation purposes.

I will provide several picts below showing the original source types, the generated types before / after your changes. The following images will capture both the concerns being discussed.

Original source types:

Image

Original generated types (rollup-plugin-dts) prior to your changes (with my local workaround for namespaces discussed here):

Image

Generated types after PR 352:

  • The red line indicates the missing export keyword.
  • The blue underline indicates unnecessary duplication of export names.
Image

The result in downstream usage of the nested namespace without the export keyword shows the breakage since it is no longer publicly accessible:

  • This is from a consuming project using TJSPosition.
Image

Original API docs before the change shows the API namespace:

Image

API docs after PR 352 missing the API namespace due to it not being exported / public:

Image

I am not sure why I didn't catch the above sooner in my initial code review of your changes. I might have just stopped short and reported my initial findings regarding the export import keyword change for exported interfaces without finding these issues as well.

Removing lines 596-605 will cause the tests you added to fail, but that is only because the test results expect the export keyword removed.

IMHO this change to types should not occur as it changes intention and scoping of the original types for nested namespaces.


Regarding the unnecessary invocation of duplicateExports on lines 26-28. I believe that this is entirely superfluous and can simply be removed. Removing it doesn't cause any tests to fail. Again I believe original types should not be unnecessarily modified.


Almost there! I really appreciate your PRs and efforts to get this functionality rock solid and I hope you don't mind my feedback. I have a complex series of projects that use a lot of nested namespaces.

It should be noted as of (10/15/25) when I run the tests as is testcases/externals-link fails without any modifications to the current source code in the main repo, so this is likely an unrelated failure in regard to the proposed changes above.

I believe lines 26-28 and 596-605 can be safely removed from preprocess.ts.

Glad to have more discussion / clarification.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions