Skip to content
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

fix type exports for cherry-picked imports #2370

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

jamesarosen
Copy link

@jamesarosen jamesarosen commented Feb 8, 2025

Previously, the type exports worked for imports that rely on tree-shaking:

import { SlOption } from '@shoelace-style/shoelace/dist/react';

but not for cherry-picked imports:

import SlOption from '@shoelace-style/shoelace/dist/react/option';

This fixes that by declaring the type type exports for all paths.

See #2111

NOTE the build is set up in such a way that there are some inconsistencies:

  • For themes/, utilities/, and translations/, the import is of the form @shoelace-style/shoelace/translations/ar
  • For components/, the import is of the form @shoelace-style/shoelace/components/alert/alert
  • For react/, the import is of the form @shoelace-style/shoelace/react/select and references ./dist/react/select/index.js

There aren't any tests for the React wrappers. I think if there were and if they imported from the public paths, not relative ones, they would have caught this.

Some alternate approaches we could use instead:

  • move all the React component files out of the subdirectories so they compile to dist/react/COMPONENT.{js,d.ts}
  • change the docs to say to import from '@shoelace-style/shoelace/dist/react/COMPONENT/index'

Copy link

vercel bot commented Feb 8, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview Feb 10, 2025 0:19am

@jamesarosen
Copy link
Author

jamesarosen commented Feb 9, 2025

I don't know that this fixes anything yet. I'm working on fixing the type-checking in the project so we can test it.

"Tests" added in the form of an additional tsc call.

@jamesarosen jamesarosen marked this pull request as draft February 9, 2025 18:06
@jamesarosen jamesarosen marked this pull request as ready for review February 9, 2025 22:01
@jamesarosen jamesarosen marked this pull request as draft February 9, 2025 22:04
@jamesarosen
Copy link
Author

What I'm struggling with right now:

If I run npm run build before npm run typecheck, I get errors like

Subsequent property declarations must have the same type.  Property ''sl-button'' must be of type 'SlButton', but here has type 'SlButton'.ts(2717)

I think it's because TypeScript is finding both the src/ and dist/ versions and confusing them, but I'm not sure.

If I run npm run typecheck first, then dist/ doesn't exist, so the React components aren't defined for the test.

fix type exports for cherry-picked imports

Previously, the type exports worked for imports that rely on
tree-shaking:

```ts
import { SlOption } from '@shoelace-style/shoelace/dist/react';
````

but not for cherry-picked imports:

```ts
import SlOption from '@shoelace-style/shoelace/dist/react/option';
```

This fixes that by declaring the type type exports for all paths.

It also adds a new `test/` directory with a few very high-level smoke
tests that run against the compiled `dist/` output.

See shoelace-style#2111
@jamesarosen
Copy link
Author

npm run verify now successfully checks that the exports in package.json match the actual compiled output, at least for the React exports.

Comment on lines +31 to +34
"./dist/themes/*": {
"types": "./dist/themes/*.d.ts",
"import": "./dist/themes/*.js"
},
Copy link
Author

Choose a reason for hiding this comment

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

This is a bug. It makes it impossible to

import '@shoelace-style/shoelace/dist/themes/light.css';

@jamesarosen
Copy link
Author

I started integrating with a NextJS application, following this guide. It has examples like

import { setBasePath } from "@shoelace-style/shoelace/dist/utilities/base-path.js"

and

const SlButton = dynamic(
  // Notice how we use the full path to the component. If you only do `import("@shoelace-style/shoelace/dist/react")` you will load the entire component library and not get tree shaking.
  () => import("@shoelace-style/shoelace/dist/react/button/index.js"),
  {
    loading: () => <p>Loading...</p>,
    ssr: false,
  },
);

That agrees with the existing package.json exports. It's possible that the real fix is to fix the React docs. They currently suggest

import SlButton from '@shoelace-style/shoelace/dist/react/button';

To agree with the NextJS guide, that should be

import SlButton from '@shoelace-style/shoelace/dist/react/button/index.js';

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