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 mapping in exports #1020

Merged
merged 2 commits into from
Nov 22, 2022
Merged

Fix mapping in exports #1020

merged 2 commits into from
Nov 22, 2022

Conversation

uilton-oliveira
Copy link
Contributor

Fixes #1019

  • Add correct mapping for public entrypoints.

Fixes #1019
- Add correct mapping for public entrypoints.
@vercel
Copy link

vercel bot commented Nov 22, 2022

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

Name Status Preview Updated
shoelace ✅ Ready (Inspect) Visit Preview Nov 22, 2022 at 3:56PM (UTC)

Copy link
Member

@claviska claviska left a 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",
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me test it

Copy link
Contributor Author

@uilton-oliveira uilton-oliveira Nov 22, 2022

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

Copy link
Member

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.

Copy link
Member

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';

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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!

Copy link
Member

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';
@claviska claviska merged commit 4f7d915 into shoelace-style:next Nov 22, 2022
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.

Error with new exports field on package.json
2 participants