chore: setIsSelfAccepting param to isSelfAccepting #10960
Conversation
| { | ||
| isSelfAccepting: | ||
| canSkipImportAnalysis(url) || forceSkipImportAnalysis | ||
| ? false | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
I'd prefer to avoid an extra object allocation for now.
Btw, I handled this change in #7477. Maybe we can close this PR in favor of that, since it's not an urgent change?
There was a problem hiding this comment.
I don't think the extra allocation has any perf implications given everything else that is run when we create a new ModuleNode.
But seeing the errors in CI, the ModuleNode is a public API, and the setIsSelfAccepted param is documented at least in the code. So this is a small breaking change. I think forcing everybody to set false when creating a node is too disruptive. I'll close this PR, but I think we should leave setIsSelfAccepted untouched in your PR too. I don't think it is worth the disruption, I thought before that it would be something internal to core only.
There was a problem hiding this comment.
It may be a public API, but it's also undocumented. If anything, we should add private to the constructor and see if anyone complains in the next alpha. 🤞 If someone opens an issue about it, we can discuss the use case, but I'm guessing that ensureEntryFromResolved, ensureEntryFromUrl, and createFileOnlyEntry cover them all.
There was a problem hiding this comment.
Ah I just realized that setIsSelfAccepted isn't only a constructor argument. You might be right that it's just too dangerous to change the default.
Description
See #8898 (comment) for context
What is the purpose of this pull request?