-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor: remove unused commonjs plugin #1582
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import React, { useId, useState } from 'react'; | ||
|
||
import { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import React from 'react'; | ||
|
||
import { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import React, { useId, useState } from 'react'; | ||
|
||
import { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
|
||
All notable changes to this project will be documented in this file. See [standard-version](https://github.com/conventional-changelog/standard-version) for commit guidelines. | ||
|
||
## [12.0.0-alpha.1](https://github.com/chanzuckerberg/edu-design-system/compare/v12.0.0-alpha.0...v12.0.0-alpha.1) (2023-04-04) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oop alpha publish changelogs snuck in here but shouldn't matter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked back and saw a few other older builds included alpha stuff in the change log. This shouldn't interfere with any releases, but makes the change log slightly weird |
||
## [12.0.0-alpha.0](https://github.com/chanzuckerberg/edu-design-system/compare/v11.0.0...v12.0.0-alpha.0) (2023-04-04) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
{ | ||
"name": "@chanzuckerberg/eds", | ||
"version": "12.0.0-alpha.0", | ||
"version": "12.0.0-alpha.1", | ||
"description": "The React-powered design system library for Chan Zuckerberg Initiative education web applications", | ||
"author": "CZI <[email protected]>", | ||
"homepage": "https://github.com/chanzuckerberg/edu-design-system", | ||
|
@@ -109,7 +109,6 @@ | |
"@commitlint/cli": "^17.5.1", | ||
"@commitlint/config-conventional": "^17.4.4", | ||
"@geometricpanda/storybook-addon-badges": "^1.1.1", | ||
"@rollup/plugin-commonjs": "^24.0.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused plugin, tsconfig handles for us |
||
"@rollup/plugin-node-resolve": "^15.0.1", | ||
"@rollup/plugin-typescript": "^11.0.0", | ||
"@size-limit/file": "^8.2.4", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import React, { | ||
type ReactNode, | ||
useState, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import React, { | ||
useCallback, | ||
useState, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import clsx from 'clsx'; | ||
import { clsx } from 'clsx'; | ||
import type { | ||
ReactNode, | ||
KeyboardEvent, | ||
|
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.
Weird. What error gets thrown with default export + cjs builds?
Whether it's named or not shouldn't matter.
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.
Same question; why is
clsx
having this issue?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.
@booc0mtaco paired on this and saw that
<Link>
is being used in a cjs build fromremix-utils
whichrequires
clsx, which I thought would pull themain
build ofclsx
according to its package.json but instead uses themodule
build. Removing thatmodule
line in clsx's package json in edu-stack allows it to resolve to themain
build instead and works fine with the default export, implying this issue could arise from import/require resolution within edu-stackThere 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.
Ahhhhhh, the link to remix-utils might be a clue.
I wonder if remix-utils/Link is importing the ESM version of EDS somehow, which then tries to load the ESM versino of clsx? 🤔
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.
remix-utils/Link is using the CJS version, but
How about building both cjs and esm for remix-utils and using conditional exports for it?: aka
This works on edu-stack