-
-
Notifications
You must be signed in to change notification settings - Fork 851
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 mapping in exports #1020
Fix mapping in exports #1020
Conversation
Fixes #1019 - Add correct mapping for public entrypoints.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but one question about a possible dup.
package.json
Outdated
@@ -13,7 +13,13 @@ | |||
".": { | |||
"types": "./dist/shoelace.d.ts", | |||
"import": "./dist/shoelace.js" | |||
} | |||
}, | |||
"./shoelace.js": "./dist/shoelace.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be here? Seems like a dup of line 15. Can either be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module not found: Error: Package path ./shoelace.js is not exported from package C:\Programacao\platform-cortex\modules\competitiva\frontend\node_modules\@cortex-intelligence\design-system (see exports field in C:\Programacao\platform-cortex\modules\competitiva\frontend\node_modules\@cortex-intelligence\design-system\package.json)
Does not work without it... the import in the dot block probably is a typescript thing... not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand it correctly, the .
export will pull from dist/shoelace.js
:
import { thing } from '@shoelace-style/shoelace';
And the ./shoelace.js
export will also pull from the same place.
import { thing } from '@shoelace-style/shoelace/shoelace.js';
The latter seems redundant, but I'm not sure which is better. The only time one should pull from shoelace.js
is when you want the entire library to load. Depending on the bundler, tree shaking may not work as expected (looking at you, webpack!)
So I wonder if we should discourage pulling from @shoelace-style/shoelace
without specifying a specific file, be it shoelace.js
(all components) or specific components/utils.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not work without it... the import in the dot block probably is a typescript thing...
Does it fail using:
import { thing } from '@shoelace-style/shoelace';
or this one?
import { thing } from '@shoelace-style/shoelace/shoelace.js';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good. This will mean that CDN paths will be out of sync since they still need the dist
for jsDelivr, but once we move to our own CDN we can remap that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm actually not sure if CDN users will be able to remove dist
now too. Might have to do a test release. The best hint I can find whether JSDelivr supports this or not is here:
jsdelivr/jsdelivr#18298 (comment)
Has anyone tried this before with jsDelivr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on that comment, it should probably work... but never tested it... maybe open a Discussion to bring more visibility to this question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got some clarification:
The exports field isn't considered in that case but you can of course load any file directly (via its full URL). However, we don't do any rewriting of imports in that case so it would only work if you don't have imports of other pages or use resolved URLs for them.
The previous comment was intended for esm.run, which has a known issue with singletons so I can't use that at the moment.
After looking through the docs, I think leaving dist
in both paths for now will be best to avoid confusion. When Shoelace moves to its own CDN, which is something I'd like to do, we can probably remap it there and remove dist
from both at the same time.
I'll make this update shortly. Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 6c7d7f4.
Should still works fine while importing this way: import { thing } from '@shoelace-style/shoelace';
Fixes #1019