-
Notifications
You must be signed in to change notification settings - Fork 5
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
Web-components: Prepare project for publication #37
Conversation
7b43107
to
8d4ed20
Compare
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.
@heymatthenry thanks for reorganizing this.
Is there a sandbox branch I can test this on or similar? I've tried installing via NPM [npm i "uswds/uswds-next#mh/configure-package"
], but running into issues referencing module in both AstroJS and Vite - quick test branch.
Results in error:
3:27:58 PM [vite] Internal server error: Failed to resolve entry for package "@uswds/web-components". The package may have incorrect main/module/exports specified in its package.json.
package.json
Outdated
@@ -43,6 +52,7 @@ | |||
"http-server": "^14.1.1", | |||
"jsdom": "^24.1.0", | |||
"prettier": "^3.3.2", | |||
"rimraf": "^6.0.1", |
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.
suggestion (non-blocking): NPM shows the size of this dependency is 281 kB
1. A small NPM script could replace this in the future - Node.js — Remove a folder.
Footnotes
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 for this! I had rimraf
in there for cross-platform support but leaning on the node utility for that directly is a good call.
package.json
Outdated
"import": "./dist/components/index.js", | ||
"default": "./dist/components/index.js" | ||
}, | ||
"./components/*": "./dist/components/*.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.
suggestion (non-blocking): Giving users an entry point to components source would be beneficial.
@mejiaj I think what's going on is that npm installing from the branch doesn't trigger the |
Hey there @heymatthenry! I'm having trouble installing this branch on a project. Hey there Matt! Trying to test #37 Unfortunately, I haven't been able to install the branch with the required The script mentioned in the PR results in a pathing error: ❯ npm i uswds/uswds-next/tree/mh/configure-package
npm error code ENOENT
npm error syscall open
npm error path /Users/cmahoney/web/uswds-sandbox/uswds/uswds-next/tree/mh/configure-package/package.json
npm error errno -2
npm error enoent Could not read package.json: Error: ENOENT: no such file or directory, open '/Users/cmahoney/web/uswds-sandbox/uswds/uswds-next/tree/mh/configure-package/package.json'
npm error enoent This is related to npm not being able to find a file.
npm error enoent
npm error A complete log of this run can be found in: /Users/cmahoney/.npm/_logs/2024-08-15T14_43_45_664Z-debug-0.log Trying to install using the github URL gives me a pathing error. ❯ npm install "https://github.com/uswds/web-components/tree/mh/configure-package"
npm error code 1
npm error The git reference could not be found
npm error command git --no-replace-objects checkout mh
npm error error: pathspec 'mh' did not match any file(s) known to git
npm error A complete log of this run can be found in: /Users/cmahoney/.npm/_logs/2024-08-15T14_49_12_478Z-debug-0.log I'm able to install it with the hash branch selector, but I'm missing the
|
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.
- Got
<usa-link>
to work in my sample vite project vianpm link
. - Confirmed issue @mahoneycm was having that installing branch doesn't include
scripts
directory.npm i "uswds/web-components#mh/configure-package"
Using Banner triggers this console error:
lit.js?v=61b418f9:1246 Uncaught (in promise) TypeError: currentDirective._$initialize is not a function
at resolveDirective (lit.js?v=61b418f9:1246:24)
at AttributePart._$setValue (lit.js?v=61b418f9:1668:13)
at TemplateInstance._update (lit.js?v=61b418f9:1318:16)
at _ChildPart._commitTemplateResult (lit.js?v=61b418f9:1524:16)
at _ChildPart._$setValue (lit.js?v=61b418f9:1410:12)
at render (lit.js?v=61b418f9:1858:8)
at R.update (lit.js?v=61b418f9:1919:24)
at R.performUpdate (lit.js?v=61b418f9:732:14)
at R.scheduleUpdate (lit.js?v=61b418f9:676:25)
at R.__enqueueUpdate (lit.js?v=61b418f9:652:25)
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.
Minor suggestion on import otherwise LGTM.
"import": "./dist/components/index.js", | ||
"default": "./dist/components/index.js" | ||
}, | ||
"./components/*": "./dist/components/*.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.
question: Should we remove components
to shorten the import?
"./components/*": "./dist/components/*.js", | |
"./*": "./dist/components/*.js", |
// Before this change user imports via:
- import "@uswds/web-components/components/usa-banner";
+ import "@uswds/web-components/usa-banner";
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.
This is a good suggestion, and I've been thinking about it for a bit. The main reasons I included it are clarity around intent and leaving room to have other named exports like, say, /styles
or /fonts
. Maybe a YAGNI thing? I don't have strong feelings about it, beyond noting that having a /components
export seems to be a common practice among other component-based design systems.
Re: banner errorSo far found two issues:
Issue unintentionally closed when I submitted the comment. Re-opened |
@mejiaj Another data point on the banner error: it's not just vite. I tried it with |
Summary
Configured the project to allow publication of the
@uswds/web-components
package.Problem statement
Prior to this PR, the repo was set up as a multi-package monorepo, where each component had its own build configuration. This setup may have been helpful if/when we begin to distribute the components on a CDN, but it also had the potential to introduce a lot of complexity and duplication.
Solution
This PR reconfigures the project to build a single package. As a result of this, end users who install the package via npm will be able to install a single package and then import individual components from that package as needed. Eventually we will need to come up with a separate build process for publishing to a CDN, but for now the single package approach will be simpler for USWDS users as well as the USWDS team.
Major changes
The main changes this PR makes are to:
package.json
; andclean
step to the build process to delete the previous build before creating a new oneTesting and review
The package isn't yet on npm, but can be installed from this branch with:
If you install it that way into a project, you should be able to import all of the components into a page using:
or import individual components:
Installing the package from github should "just work" now. I installed it as described above and was able to add the components to an off-the shelf Vite app.
Important
Depending on where you install this, you might get an error like
currentDirective._$initialize is not a function
in the browser console. For instance, I get this when adding the package to an Astro site running in dev mode. It seems to be due to having multiple lit versions on the site (Astro's and USWDS's) both running in development mode.