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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd18e3f
Fix mapping in exports
uilton-oliveira Nov 22, 2022
047c12b
Remove shoelace.js from from exports list
uilton-oliveira Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

"./themes/*": "./dist/themes/*",
"./components/*": "./dist/components/*",
"./utilities/*": "./dist/utilities/*",
"./react/*": "./dist/react/*",
"./translations/*": "./dist/translations/*"
},
"files": [
"dist"
Expand Down