Convert ambient module declarations to proper TypeScript module files#3189
Open
robigan wants to merge 9 commits into
Open
Convert ambient module declarations to proper TypeScript module files#3189robigan wants to merge 9 commits into
robigan wants to merge 9 commits into
Conversation
…iles Agent-Logs-Url: https://github.com/robigan/design-system-react/sessions/d178f6f0-ab98-4a56-9ab1-67574ac1d872 Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
…-declarations Convert ambient module declarations to proper TypeScript module files
|
Thanks for the contribution! Before we can merge this, we need @robigan to sign the Salesforce Inc. Contributor License Agreement. |
Agent-Logs-Url: https://github.com/robigan/design-system-react/sessions/2ac66c8a-afca-4f9d-8e31-9b020ee6ea08 Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/robigan/design-system-react/sessions/09accf4f-0490-4f9f-be53-799dcd9f74c2 Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/robigan/design-system-react/sessions/5557824c-6f41-44e1-8a29-7baedd6859d8 Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
fix: add `declare` modifier to top-level function declarations in .d.ts files
Agent-Logs-Url: https://github.com/robigan/design-system-react/sessions/2bf91751-e078-4dbd-97c2-aa0c89409617 Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
…espace-error Co-authored-by: robigan <35210888+robigan@users.noreply.github.com>
Qualify JSX namespace with React prefix in all .d.ts files
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This properly fixes TypeScript declarations for components imported with the quick setup usage pattern as demonstrated in the README
design-system-react/README.md
Lines 30 to 38 in 825de01
The problem lies in the fact that ambient module declarations are exclusively matched against the import specifier exactly.
import { Button } from '@salesforce/design-system-react';won't match againstdeclare module '@salesforce/design-system-react/components/button'because the import specifiers do not match exactly.import Button from '@salesforce/design-system-react/components/button';would instead work.There is also a problem for ESM module users whereby the declaration files in the
/moduledirectory do not contain these prefixes and fail to resolve the matching TS declaration; by making the ambient modules proper ESM/TS modules, the imports become relative and no longer rely on matching the absolute path. Thus, properly fixing #3113.Motivation
It's true that one could continue with the advanced usage pattern described in the README, but I would argue this is anti DX.
One, a package should be a black box when developing software, I'd say that a developer shouldn't have to know the internal structure of a package for them to use that package and reap the DX benefits technology stacks like TypeScript offer. I should just be able to import the stuff I need from the import specifier of the module and be on my merry way.
Two, in my personal experience, weird bugs arise, and some systems like Yarn PnP get pissy, when importing internal structures from a module.
Three, it seems that although there is little activity in this repository in the last few months, this repo can still be a platform for the Salesforce team to push SLDS 2 React components to, by pushing for users of this library to use the quick setup usage pattern, it gives maintainers flexibility to change repository structure how they see fit without breaking usage of this library.
Four, I'd argue that the "quick setup" usage pattern is the TypeScript best practice, when writing code that uses this library, tools like Visual Studio Code autoimport missing components using the module import specifier. Having to manually change to the advanced usage pattern can be quite cumbersome.
Thanks for listening to my Ted talk.
Large Language Model declaration & Additional Description
This PR was mostly created using LLMs to automate the application of this proposed fix to 183 qualifying ambient module declarations.
NB: This PR is marked as Draft as I've identified an issue that TS complains about that I plan to fix. The PR is being opened for public review whilst I get it ready.Issues have been fixed and tested for.How I plan to prevent AI slop
I have currently checked about 10% of the changed files and validated that GitHub Copilot has properly applied the idea/modifications I had in mind. I am testing my fork on an enterprise project I am working for based on SF Marketing Cloud Engagement to properly validate that the new TS declarations work in an enterprise project using this library. I plan to manually verify all 183 changed files once I deem this PR ready for review.
How I plan to be transparent about LLM usage
The PR on my fork can be seen at robigan#1 whereby Copilot's own internal review process can be found. The Copilot session can also be found in the PR by clicking "View Session" or by following this link (I am unsure if GH allows others to see the session, but I have linked anyways just in case). Within the Copilot session can be found the prompt I used to make the changes. I am also embedding the prompt just below for transparency.
Edit: The rest of my changes can be seen in robigan#2 and robigan#3
Prompt Used
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test). (NB: See comments)Tests have been added for new props to prevent regressions in the future. See readme.No new props have been addedREVIEWER checklist (do not remove)
components/component-docs.jsontests.Required only if there are markup / UX changes
last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.