-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Build: Bundle lib/core-server with ts-up #19278
Conversation
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
@@ -1,5 +1,5 @@ | |||
import { dirname, join } from 'path'; | |||
import { copy, writeFile, remove } from 'fs-extra'; |
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.
What's happening with all these changes @ndelangen? Does tsup not like named imports in some cases?
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.
I think what happening is that node now runs in actual ESM mode, or esbuild-register is mimicking that mode more accurately, and fs-extra
isn't actually ESM.
@@ -1,6 +1,8 @@ | |||
// eslint-disable-next-line @typescript-eslint/triple-slash-reference | |||
/// <reference path="./typings.d.ts" /> | |||
|
|||
// |
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.
?
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.
I had a commit that was corrupt on CI 😖 I needed to commit something to kick it into action. Will remove.
const defaultFavIcon = path.join( | ||
path.dirname(require.resolve('@storybook/core-server/package.json')), | ||
'public', | ||
'favicon.ico' | ||
); |
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.
const defaultFavIcon = path.join( | |
path.dirname(require.resolve('@storybook/core-server/package.json')), | |
'public', | |
'favicon.ico' | |
); | |
const defaultFavIcon = path.resolve( | |
require.resolve('@storybook/core-server/package.json'), './public/favicon.ico') |
Converted into draft, this seems to be a very pivotal package, not easy to convert. I might drop this for now, since I need to work on other stuff. |
related: #18732