-
Notifications
You must be signed in to change notification settings - Fork 15
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
LG-3757: Create bundler-ready module entrypoint #165
Conversation
The Snyk advisory is arguably not worth concerning over, but it can't be resolved without upgrading USWDS, incidentally resolved by #159. |
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.
LGTM, one question
Also, should have mentioned earlier that the "visual-regression" failure is expected: It's catching the new content added to the "Usage" page. 😅 At some point I'd like to revisit this to ideally report as a comment and not as a build failure. |
9672158
to
3cf4c73
Compare
**Why**: As a developer using the login.gov Design System, I expect that I can use identity-style-guide as a proper NPM package, so that I can deduplicate shared dependencies (see LG-3716) and take advantage of dead code elimination to reduce bundle size by omitting unused components.
**Why**: Preserve backward-compatibility while renaming source to reflect auto setup behavior
**Why**: Easier integration into existing CI setup, more consistent build artifacts
**Why**: Lint includes import validation. Since there are internal references to built package, must be prebuilt for lint.
**Why**: Changes in #167 bypass Make for running lint, but in any case, the package needs to be built.
30a0011
to
db52eee
Compare
Visual regression shows small adjustment of padding in the header. It looks that this may have been some flakiness in the rebuild from #159, since it looks more correct in the "changes", and the "changes" are reflected even in what I see in the |
**Why**: When using the ES module entrypoint, CommonJS "module.exports" and "require" are invalid. This should only be an issue when using the ES module entrypoint `build/esm/index.js`, and not an issue for CommonJS usage (`build/cjs/index.js`) or the precompiled copy (`dist/assets/js/main.js`). This was likely missed due to merge timing between #166 and #165.
**Why**: When using the ES module entrypoint, CommonJS "module.exports" and "require" are invalid. This should only be an issue when using the ES module entrypoint `build/esm/index.js`, and not an issue for CommonJS usage (`build/cjs/index.js`) or the precompiled copy (`dist/assets/js/main.js`). This was likely missed due to merge timing between #166 and #165.
Why: As a developer using the login.gov Design System, I expect that I can use identity-style-guide as a proper NPM package, so that I can deduplicate shared dependencies (see LG-3716) and take advantage of dead code elimination to reduce bundle size by omitting unused components.
Changelog:
Usage Documentation: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-lg-3716-package-entry/usage/#use-as-a-javascript-package
History:
#148 added a main to the package's package.json, which at least allows for a developer to import the package. However, because this points to a precompiled distributable asset, dependencies cannot be deduplicated.
Impact:
Preliminary testing in
identity-idp
reveals bundle size reduction of 88% gzipped (451kb to 56.3kb), largely attributed to elimination of unusedzxcvbn
dependency.