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

Build: Bundle lib/api with ts-up #19269

Merged
merged 4 commits into from
Oct 3, 2022
Merged

Build: Bundle lib/api with ts-up #19269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79c58e6
migrate lib/api to be built using tsup
ndelangen Sep 27, 2022
54673a4
fix build issues after bundling lib/api
ndelangen Sep 27, 2022
f21aca6
fix broken reference to dist
ndelangen Sep 27, 2022
c98bbec
maybe fix the unit tests this way
ndelangen Sep 27, 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
1 change: 1 addition & 0 deletions code/addons/interactions/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
"devDependencies": {
"@storybook/jest": "^0.0.10",
"@storybook/testing-library": "0.0.14-next.0",
"@types/node": "^14.14.20 || ^16.0.0",
"formik": "^2.2.9",
"typescript": "~4.6.3"
},
Expand Down
2 changes: 2 additions & 0 deletions code/addons/interactions/src/preset/preview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/// <reference types="node" />

import { addons } from '@storybook/addons';
import { FORCE_REMOUNT, STORY_RENDER_PHASE_CHANGED } from '@storybook/core-events';
import type {
Expand Down
3 changes: 3 additions & 0 deletions code/lib/addons/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
"@storybook/theming": "7.0.0-alpha.34",
"global": "^4.4.0"
},
"devDependencies": {
"@types/webpack-env": "^1.16.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a bug with NX where it builds dependents without the --optimized flag

That might be the cause of a few issues... one of those issues I got fixed by adding this, but it's not really what i want to do..

I've reached out to Katarina already

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain a little more why that leads to needing to add this? I'm not really following..

},
"peerDependencies": {
"react": "^16.8.0 || ^17.0.0 || ^18.0.0",
"react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0"
Expand Down
1 change: 1 addition & 0 deletions code/lib/addons/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import global from 'global';

import type { ReactElement } from 'react';
import { Channel } from '@storybook/channels';
import { SET_CONFIG } from '@storybook/core-events';
Expand Down
2 changes: 1 addition & 1 deletion code/lib/addons/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export interface StoryApi<StoryFnReturnType = unknown> {
}

export interface ClientStoryApi<StoryFnReturnType = unknown> {
storiesOf(kind: StoryKind, module: NodeModule): StoryApi<StoryFnReturnType>;
storiesOf(kind: StoryKind, module: any): StoryApi<StoryFnReturnType>;
addDecorator(decorator: DecoratorFunction<StoryFnReturnType>): StoryApi<StoryFnReturnType>;
addParameters(parameter: Parameters): StoryApi<StoryFnReturnType>;
}
Expand Down
27 changes: 23 additions & 4 deletions code/lib/api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,22 @@
"url": "https://opencollective.com/storybook"
},
"license": "MIT",
"main": "dist/cjs/index.js",
"module": "dist/esm/index.js",
"types": "dist/types/index.d.ts",
"exports": {
".": {
"require": "./dist/index.js",
"import": "./dist/index.mjs",
"types": "./dist/index.d.ts"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this export map is incorrect - the types condition will never be matched, since import or require will always be matched first (so we'll always be looking at the default location for the declaration file - adjacent to the source file). Moreover, you very likely want a dedicated declaration file for each of the .js and .mjs entrypoints, so TS knows the correct module format of the import for analysis. So you want something more like

  "exports": {
    ".": {
      "require": { "types": "./dist/index.d.ts", "default": "./dist/index.js" },
      "import": { "types": "./dist/index.d.mts", "default": "./dist/index.mjs" }
    },
    "./shortcut": {
      "require": { "types": "./dist/shortcut.d.ts", "default": "./dist/shortcut.js" },
      "import":  { "types": "./dist/shortcut.d.mts", "default": "./dist/shortcut.mjs" }
    },
    "./package.json": "./package.json"
  },

Naturally, you'll need those .d.mts files to actually exist.

Copy link
Member Author

@ndelangen ndelangen Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this comment!

Moreover, you very likely want a dedicated declaration file for each of the .js and .mjs entrypoints

Can you explain why? what would be the difference in content of these 2 files?
What does typescript use the "correct module format of the import for analysis" for?

so we'll always be looking at the default location for the declaration file - adjacent to the source file

So this is why it works out in the end, right?
Could we simply remove the "types" line, then?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why? what would be the difference in content of these 2 files?

The .d.ts file will be recognized as representing a common js module, while the .d.mts file will be recognized as representing a ecmascript module, which have a complex non-transparent interop scheme in node. Notably, it's an error to load esm from a common js module via require (and this can't be papered over with an import helper), so detecting this in advance requires source-accurate declaration file module format indicators.

The index.d.mts file can likely be export * from "./index.js"; if the named exports are meant to be similar. If there's meant to be a default member, you may need that, too.

What does typescript use the "correct module format of the import for analysis" for?

Knowing if the default import from an es module should be the whole module (because the target is cjs), knowing if the import should be an error (because the target is esm and the require'ing module is cjs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for taking the time to explain of of this to me (and others reading)!

Would you be able to assist us with this migration?

},
"./shortcut": {
"require": "./dist/shortcut.js",
"import": "./dist/shortcut.mjs",
"types": "./dist/shortcut.d.ts"
},
Comment on lines +27 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something addons use?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib/ui uses it

"./package.json": "./package.json"
},
"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "dist/index.d.ts",
"files": [
"dist/**/*",
"README.md",
Expand All @@ -29,7 +42,7 @@
],
"scripts": {
"check": "../../../scripts/node_modules/.bin/tsc --noEmit",
"prep": "node ../../../scripts/prepare.js"
"prep": "../../../scripts/prepare/bundle.ts"
},
"dependencies": {
"@storybook/channels": "7.0.0-alpha.34",
Expand Down Expand Up @@ -64,5 +77,11 @@
"publishConfig": {
"access": "public"
},
"bundler": {
"entries": [
"./src/index.tsx",
"./src/shortcut.ts"
]
},
"gitHead": "fc90fc875462421c1faa35862ac4bc436de8e75f"
}
2 changes: 1 addition & 1 deletion code/lib/api/shortcut.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export * from './dist/esm/lib/shortcut';
export * from './dist/shortcut';
1 change: 1 addition & 0 deletions code/lib/api/src/shortcut.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './lib/shortcut';
ndelangen marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions code/lib/ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
"@storybook/semver": "^7.3.2",
"@storybook/theming": "7.0.0-alpha.34",
"@testing-library/react": "^11.2.2",
"@types/node": "^14.0.10 || ^16.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't run in the node context, right? What node types are we using?

"@types/webpack-env": "^1.16.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we build these with esbuild, not webpack? Is it weird to depend on webpack env types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module.hot.accept does not exist in node, that's a webpack runtime thing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use that in lib/ui???

"copy-to-clipboard": "^3.3.1",
"downshift": "^6.0.15",
"enzyme": "^3.11.0",
Expand Down
12 changes: 5 additions & 7 deletions code/lib/ui/src/containers/notifications.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import React, { FC } from 'react';

import { Consumer, Combo, useStorybookApi } from '@storybook/api';
import type { Combo } from '@storybook/api';
import { Consumer } from '@storybook/api';

import NotificationList from '../components/notifications/NotificationList';

export const mapper = ({ state }: Combo) => {
const { clearNotification } = useStorybookApi();
const { notifications } = state;

const mapper = ({ state, api }: Combo) => {
return {
notifications,
clearNotification,
notifications: state.notifications,
clearNotification: api.clearNotification,
};
};

Expand Down
2 changes: 2 additions & 0 deletions code/lib/ui/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// eslint-disable-next-line @typescript-eslint/triple-slash-reference
/// <reference path="./typings.d.ts" />
/// <reference types="node" />
/// <reference types="webpack-env" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: #19269 (comment)


import global from 'global';
import React, { FC } from 'react';
Expand Down
4 changes: 4 additions & 0 deletions code/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7018,6 +7018,7 @@ __metadata:
"@storybook/jest": ^0.0.10
"@storybook/testing-library": 0.0.14-next.0
"@storybook/theming": 7.0.0-alpha.34
"@types/node": ^14.14.20 || ^16.0.0
formik: ^2.2.9
global: ^4.4.0
jest-mock: ^27.0.6
Expand Down Expand Up @@ -7355,6 +7356,7 @@ __metadata:
"@storybook/csf": 0.0.2--canary.0899bb7.0
"@storybook/router": 7.0.0-alpha.34
"@storybook/theming": 7.0.0-alpha.34
"@types/webpack-env": ^1.16.0
global: ^4.4.0
peerDependencies:
react: ^16.8.0 || ^17.0.0 || ^18.0.0
Expand Down Expand Up @@ -9382,6 +9384,8 @@ __metadata:
"@storybook/semver": ^7.3.2
"@storybook/theming": 7.0.0-alpha.34
"@testing-library/react": ^11.2.2
"@types/node": ^14.0.10 || ^16.0.0
"@types/webpack-env": ^1.16.0
copy-to-clipboard: ^3.3.1
downshift: ^6.0.15
enzyme: ^3.11.0
Expand Down