Skip to content

[Refactor] Stop rerendering directives, inject imports instead #357

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ckwalsh
Copy link

@ckwalsh ckwalsh commented May 2, 2025

Summary:
This commit changes how the sorted imports are combined with the
original source.

Prior to this commit, all ImportDeclaration nodes and their leading
comments, plus any InterpreterDirective and Directive nodes, were
extracted from the original code and re-rendered using babel. The
rendered nodes were then concatenated with the original source with
those nodes removed to produce the updated source.

This approach safely protected against functional changes, but
removed newlines around comments near the beginning of the file when
the first node of the original source was an ImportDeclaration, as
babel does not preserve whitespace when rendering content.

If a user has configured a plugin that attempts to manage comments
and/or whitespace near the top of the file, such as auto-inserting a
license header (as I am trying to do), this results in conflicts /
formatting churn.

This commit does not directly resolve this incompatibility, however
it better prepares the codebase for a plugin option to be added that
can resolve the issue.

Test Plan:
yarn install && yarn run test --all

Note that one snapshot was changed by this commit where a newline was
changed, acting as an effective example of how the original approach
could affect whitespace in the re-rendered portion of the file.

Stack created with Sapling. Best reviewed with ReviewStack.

ckwalsh added 4 commits May 2, 2025 01:56
Summary:
```
$ npx prettier --write .
```

Manually inspected changes, and reverted changes to examples, package.json, and tsconfig.json

Test Plan: doitlive
Summary:
This logic can hide breakages and cause tests to pass when the
underlying logic is actually broken. I encountered these hidden
breakages while working on some further feature changes, and figured
it was appropriate to fix this first.

As all existing tests pass without the try/catch, this change is
effectively a no-op.

Test Plan:
`yarn install && yarn run test ./tests/`
Summary:
This change replaces the existing RegExp replace() logic with
concatenated string slices. This avoids reallocation the result
string for each node replacement, replacing it with string slice
operations (which are implemented as O(1) string views within v8) and
a single .join(''), which can be optimized by the runtime to a single
allocation.

This probably won't make a noticable difference, but the change also
simplifies some further feature work I am attempting to add.

Test Plan:
`yarn install && yarn run test --all`
Summary:
This commit changes how the sorted imports are combined with the
original source.

Prior to this commit, all ImportDeclaration nodes and their leading
comments, plus any InterpreterDirective and Directive nodes, were
extracted from the original code and re-rendered using babel. The
rendered nodes were then concatenated with the original source with
those nodes removed to produce the updated source.

This approach safely protected against functional changes, but
removed newlines around comments near the beginning of the file when
the first node of the original source was an ImportDeclaration, as
babel does not preserve whitespace when rendering content.

If a user has configured a plugin that attempts to manage comments
and/or whitespace near the top of the file, such as auto-inserting a
license header (as I am trying to do), this results in conflicts /
formatting churn.

This commit does not directly resolve this incompatibility, however
it better prepares the codebase for a plugin option to be added that
can resolve the issue.

Test Plan:
`yarn install && yarn run test --all`

Note that one snapshot was changed by this commit where a newline was
changed, acting as an effective example of how the original approach
could affect whitespace in the re-rendered portion of the file.
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.

1 participant