Skip to content

fix: wrong activeDocument.createEl() calls#174

Merged
saberzero1 merged 4 commits into
obsidianmd:masterfrom
mnaoumov:createEl-fix
Jul 2, 2026
Merged

fix: wrong activeDocument.createEl() calls#174
saberzero1 merged 4 commits into
obsidianmd:masterfrom
mnaoumov:createEl-fix

Conversation

@mnaoumov

@mnaoumov mnaoumov commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the prefer-create-el ESLint rule to avoid generating broken Obsidian DOM-helper calls (notably around activeDocument) and expands handling to cover Document-typed variables, with accompanying test coverage.

Changes:

  • Update the rule to classify “Document-like” targets and rewrite activeDocument.* calls to activeWindow.*.
  • Add new invalid/valid test cases to ensure non-Document lookalikes aren’t flagged and to cover Document-typed variable rewrites (doc.win.*).
  • Refactor replacement generation to use a shared target classifier (prefix + autofix capability).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
tests/preferCreateEl.test.ts Adds coverage for non-Document objects and Document-typed variable rewrites; updates expected outputs for activeDocument to use activeWindow.
lib/rules/preferCreateEl.ts Introduces target classification (activeDocumentactiveWindow, Document-typed objects → .win.*) and applies it across createElement/createElementNS/createDocumentFragment checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/preferCreateEl.test.ts Outdated
Comment thread tests/preferCreateEl.test.ts Outdated
Comment thread tests/preferCreateEl.test.ts Outdated
Comment thread tests/preferCreateEl.test.ts Outdated
Comment thread lib/rules/preferCreateEl.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread lib/rules/preferCreateEl.ts Outdated
Comment thread lib/rules/preferCreateEl.ts
@saberzero1

Copy link
Copy Markdown
Collaborator

Considering that we have decided that we're moving document/activeDocument to window/activeWindow, is the extensive refactor in this PR necessary? Or can it be consolidated to updating the current autofix to apply the window/activeWindow instead?

@mnaoumov

mnaoumov commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Yes, I think all the cases are reasonable here. Do you see any unit tests that you think are unreasonable?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@saberzero1

saberzero1 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Yes, I think all the cases are reasonable here. Do you see any unit tests that you think are unreasonable?

I ask, because you don't just seem to just update document/activeDocument to window/activeWindow, you also seem to refactor the autofix logic.

This implies you're addressing something beyond the document/activeDocument issue with .create*() you raised in the linked bug report.

This is fine if there are additional issues in this rule that you're addressing in this changeset. My question is precisely whether that is the case, because if that is not the case, the changeset is touching more than it claims to fix. However, if you identified additional issues and addressed them in this changeset, then the changes are understandable.

Additional meaningful unit test cases are fine regardless.

@mnaoumov

mnaoumov commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Current version turns valid activeDocument.createElement() into activeDocument.createEl(), which is currently invalid due to the bug in obsidian API. Also I added support for potential doc.createElement(), where doc is some variable. All of those fixes could be useful in some user code.

Once the referenced obsidian API bug is fixed, this PR becomes redundant

@saberzero1

Copy link
Copy Markdown
Collaborator

Current version turns valid activeDocument.createElement() into activeDocument.createEl(), which is currently invalid due to the bug in obsidian API. Also I added support for potential doc.createElement(), where doc is some variable. All of those fixes could be useful in some user code.

Once the referenced obsidian API bug is fixed, this PR becomes redundant

I assume even if they address the bug today, there is going to be a significant timeframe until adoption.

I'll run some verifications and create a new release after this has been merged. Thanks for the PR.

@saberzero1
saberzero1 merged commit f091909 into obsidianmd:master Jul 2, 2026
4 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 2, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants