Fix generated ESM, revamp build system #113

hobbes7878 · 2023-07-19T22:37:51Z

Attempt to close #111.

Done:

Updates build system using @sveltejs/package.
Makes package an ES module.
Converts all the scripts to typescript and fixes any outstanding type issues.
Updates a buncha dev dependencies.
Working with storybook 7.1, updated to use Vite framework.
Fix/upgrade tests (though, drops snapshot tests in favour of using Chromatic)

📦 Published PR as canary version: 3.0.5--canary.113.4269195.0

✨ Test out this PR locally via:

npm install @storybook/addon-svelte-csf@3.0.5--canary.113.4269195.0
# or 
yarn add @storybook/addon-svelte-csf@3.0.5--canary.113.4269195.0

…ts of small upgrades to make it work.

JReinhold · 2023-07-20T12:15:49Z

This is looking great @hobbes7878 ! 🥰

It's going to be interesting to see if this will still work in a Webpack-based project, but if not, it would be okay to drop support for that, we're seeing very little usage of Storybook 7 + Webpack + Svelte + this addon.

hobbes7878 · 2023-07-20T15:15:45Z

Thanks @JReinhold. Happy if it helps.

On tests, moving the whole package to ECMAScript modules has made it a little tricky to work with Jest. Right now, I've replaced it with vitest for all the unit tests, which was easier to get going.

That leaves the snapshot tests. I wonder if Chromatic is a good enough replacement for the storyshots. I've updated this branch with that in mind for now, but happy if someone wants to give me any hints/examples how to bring those back...

socket-security · 2023-07-20T15:16:18Z

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@storybook/test-runner	0.11.0	network, filesystem, shell, environment	`+14`	11.7 MB	shilman
vitest	0.33.0	filesystem, shell, environment	`+3`	4.83 MB	oreanno
@sveltejs/package	2.2.0	None	`+0`	26.9 kB	svelte-admin
storybook	7.1.1	None	`+12`	17.1 MB	shilman
vite	4.4.7...4.4.3	None	`+0/-41`	3.32 MB	vitebot
chromatic	6.17.2...6.19.9	environment	`+0/-161`	4.11 MB	thafryer
eslint	7.32.0...7.17.0	None	`+0/-106`	3.1 MB	eslintbot
concurrently	6.5.1...8.2.0	None	`+0/-25`	121 kB	gustavohenke

🚮 Removed packages: @auto-it/released@10.36.5, @babel/cli@7.21.0, @babel/core@7.21.4, @babel/plugin-transform-runtime@7.21.4, @babel/preset-env@7.21.4, @babel/preset-react@7.18.6, @babel/preset-typescript@7.21.4, @babel/runtime@7.21.0, @storybook/addon-essentials@7.0.27, @storybook/addon-interactions@7.0.27, @storybook/addon-storyshots@7.0.27, @storybook/core-client@7.0.27, @storybook/core-server@7.0.27, @storybook/eslint-config-storybook@3.1.2, @storybook/jest@0.0.10, @storybook/svelte@7.1.1, @storybook/svelte-webpack5@7.0.27, @storybook/testing-library@0.0.13, @storybook/theming@7.1.1, @storybook/types@7.0.27, @sveltejs/vite-plugin-svelte@2.4.3, babel-jest@29.5.0, babel-loader@8.2.4, fs-extra@11.1.1, jest@29.5.0, jest-environment-jsdom@29.5.0, magic-string@0.30.0, prettier@2.8.7, react@17.0.2, react-dom@17.0.2, rimraf@3.0.2, sb@7.0.27, svelte@4.0.5, svelte-jester@2.3.2, svelte-loader@3.1.9, ts-dedent@2.2.0

socket-security · 2023-07-20T15:16:21Z

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Issue	Package	Version	Note	Source
Bin script confusion	storybook	7.1.1	Bin Script: sb	`package.json`
Shell access	@storybook/test-runner	0.11.0	Module: child_process Location: dist/test-storybook.js +1 more instances...	`package.json`
Shell access	playwright-core	1.36.2	Module: child_process Location: lib/cli/program.js +8 more instances...	@storybook/test-runner@0.11.0 via `package.json`
Uses eval	playwright-core	1.36.2	Eval Type: Function Location: lib/server/javascript.js +1 more instances...	@storybook/test-runner@0.11.0 via `package.json`
Shell access	vitest	0.33.0	Module: child_process Location: dist/chunk-install-pkg.23da664c.js +5 more instances...	`package.json`

Next steps

What is bin script confusion?

This package has multiple bin scripts with the same name. This can cause non-deterministic behavior when installing or could be a sign of a supply chain attack

Consider removing one of the conflicting packages. Packages should only export bin scripts with their name

What is shell access?

This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code.

Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced.

What is eval?

Package uses eval() which is a dangerous function. This prevents the code from running in certain environments and increases the risk that the code may contain exploits or malicious behavior.

Avoid packages that use eval, since this could potentially execute any code.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore playwright-core@1.36.2
@SocketSecurity ignore @storybook/test-runner@0.11.0
@SocketSecurity ignore vitest@0.33.0
@SocketSecurity ignore storybook@7.1.1

hobbes7878 · 2023-07-21T10:57:50Z

It's going to be interesting to see if this will still work in a Webpack-based project, but if not, it would be okay to drop support for that, we're seeing very little usage of Storybook 7 + Webpack + Svelte + this addon.

Yeah, I'm not sure, but there weren't any webpack-specific plugins in it originally, so as long as webpack can tolerate the move to ESM and this did previously work there, it should be fine??

davemackintosh · 2023-07-23T11:09:52Z

I've just installed this and I can confirm the issue is fixed for me. Thank you! 🙏

olafurw · 2023-07-24T08:25:37Z

Tested this also on a branch that had this issue. Fixes the issue for me.

ndelangen · 2023-07-25T13:10:28Z

package.json

-  "main": "dist/cjs/index.js",
-  "module": "dist/esm/index.js",
-  "browser": "dist/cjs/index.js",


You say this works in storybook?
I would expect this to be broken, but honestly I've not yet tried, so maybe I'm wrong.

What i know of the preset-loading mechanism in storybook is what when an addon-entry is added, storybook will try resolving ${name}/preset (with node).
It will do the same for manager and preview FYI.

Maybe with type: module these exports fields are actually respected/used?
If so that's great!

Yeah, type: "module" is the key there as far as I understand it, which is mostly from sindresorhus's explanation...

But yes, working in Node 16+ with Storybook 7.1+ in the repo.

src/plugins/vite-svelte-csf.ts

Co-authored-by: ilsubyeega <ilsubyeega@outlook.com>

ndelangen · 2023-07-26T12:34:01Z

src/plugins/vite-svelte-csf.ts

@@ -7,7 +7,7 @@ import { fileURLToPath } from 'url';
 import { getNameFromFilename } from '../parser/svelte-stories-loader.js';
 import { readFileSync } from 'fs';

-const parser = fileURLToPath(new URL('../parser/collect-stories.js', import.meta.url));
+const parser = fileURLToPath(new URL('../parser/collect-stories.js', import.meta.url)).replace(/\\/g, "\\\\");


I'd recommend making this very obvious why this is the way it is perhaps with a clear code-comment?, and possibly even ensure this continues working with a unit-test.

Added handling across any dynamic imports in fa5505b.

@ilsubyeega don't know if you'd be able to help with a test?

I can say the current branch works on my machine (win11-x64) with yarn test and storybook live view.
However, I'm uncertain whether the changes to presets directory are working or not.

Also, I'm not sure about creating other test tasks for this.

JReinhold

Absolutely fantastic job, I also love the way you've cleaned up the code all over the place. The "conversion" from storyshots to Chromatic is also *chef's kiss* (and Jest -> vitest), storyshots really shouldn't be used anymore.

I have left some comments in the code, and then a big issue is that the Storybook fails to build, which is evident by the Chromatic check on this PR failing, and I was able to reproduce it locally. It's failing with some CJS->ESM shenanigans in cli-table3. I was able to fix it locally by upgrading all Storybook dependencies from 7.1.0 to 7.1.1, could you try that? I'll investigate on the core end why this is.

PS. @hobbes7878 would you like to become a maintainer of this project?

package.json

src/config-loader.ts

tsconfig.json

Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>

hobbes7878 · 2023-07-29T10:59:01Z

Thanks @JReinhold. Addressed all those suggestions, I think.

I'm still seeing that cli-table3 issue with 7.1.1 Storybook in the Chromatic workflow. Storybook is building fine for me locally. Will keep investigating...

Re: maintainer, I don't mind, sure!

hobbes7878 · 2023-07-29T12:16:17Z

I have left some comments in the code, and then a big issue is that the Storybook fails to build, which is evident by the Chromatic check on this PR failing, and I was able to reproduce it locally. It's failing with some CJS->ESM shenanigans in cli-table3. I was able to fix it locally by upgrading all Storybook dependencies from 7.1.0 to 7.1.1, could you try that? I'll investigate on the core end why this is.

Looks like this isn't fixed just by updating storybook to 7.1.1. But there's something really weird going on with yarn's dependency resolution here for string-width.

I can consistently reproduce this issue based on the existence of yarn.lock.

So if yarn.lock exists and you clear out node_modules/ and re-yarn to install fresh deps from the lockfile, you get the ERR_REQUIRE_ESM error in cli-table3 requiring string-width. That's what's of course happening in the workflow, so that busts out.

If I delete yarn.lock (or change a dep so it's regenerated) and then yarn, no error.

There is no difference in the lockfiles in either the working or erroring scenarios.

OK, on further inspection of node_modules, it gets weirder.

In the working scenario, string-width is installed with version 4.2.3. In the erroring scenario, it's at 5.1.2, which is after the library converted to ESM. Again, zero differences in the lockfile in either scenario.

There are two libraries that require string-width >5 -- @isaacs/cliui^8.0.2 and wrap-ansi@^8.1.0 -- but one requires the other, so I'm putting money on @isaacs/cliui.

"@isaacs/cliui@^8.0.2":
  version "8.0.2"
  resolved "https://registry.yarnpkg.com/@isaacs/cliui/-/cliui-8.0.2.tgz#b37667b7bc181c168782259bab42474fbf52b550"
  integrity sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==
  dependencies:
    string-width "^5.1.2"
    string-width-cjs "npm:string-width@^4.2.0"
    strip-ansi "^7.0.1"
    strip-ansi-cjs "npm:strip-ansi@^6.0.1"
    wrap-ansi "^8.1.0"
    wrap-ansi-cjs "npm:wrap-ansi@^7.0.0"

The rest of the chain is @isaacs/cliui@^8.0.2 required by --> jackspeak@^2.0.3 --> glob@^10.0.0, glob@^10.2.2 --> and finally, @storybook/core-common@7.1.1 & @storybook/test-runner@^0.11.0.

Weird!

OK, I've been able to "fix" the issue locally and seems in the workflow, but to do it, I had to force yarn to resolve specific versions of string-width and wrap-ansi in 920391d.

Is this the ultimate fix for the issue? Probably not... Does it get the lib past the immediate issue? It does...

I'll leave it to wiser minds to decide if it's an acceptable solution...

JReinhold · 2023-07-29T19:38:02Z

Great work!

That sounds super strange, I have no idea why it behaves that way. Could this just be a bug in Yarn 1? I mean, it hasn't been updated for a while.

Would you be open to migrating to pnpm here and see if that automatically fixes the issue? I don't think there's any deeper integrations with yarn at play here, so it should be a simple replacement, but if it's not I'm okay with the current solution you have for now.

hobbes7878 · 2023-07-29T20:42:20Z

Not gonna lie, it took a couple tries but migrated to pnpm, which is running fine without any dep overloads. 👍

JReinhold · 2023-07-31T14:13:50Z

.github/workflows/release.yml


      - name: Create Release
        env:
          GITHUB_TOKEN: ${{ secrets.GH_TOKEN }}
          NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
        run: |
-          yarn release
+          npm run release


shouldn't all these npm run be pnpm run instead to use the correct package resolution, or am I misunderstanding something? Same for the lines in package.json?

Yeah, that was silly of me. Done in 4269195.

JReinhold · 2023-07-31T19:06:51Z

Thanks for your hard work and patience @hobbes7878!

shilman · 2023-07-31T19:09:13Z

🚀 PR was released in v3.0.5 🚀

oscard0m · 2023-07-31T19:22:44Z

It works! Thanks for the amazing job on this @hobbes7878 🌟
And thanks to the rest of participants as well
🎖️ @JReinhold
🎖️ @ilsubyeega
🎖️ @ndelangen
🎖️ @shilman
🎖️ @olafurw
🎖️ @davemackintosh

hobbes7878 added 4 commits July 19, 2023 23:31

replace build system with svelte-package. refactor for typescript. lo…

0f5bf98

…ts of small upgrades to make it work.

adds test-runner

67bc5e4

resolve merge conflicts

53f31e9

await click in interaction test

40c1152

finish tests with vitest. cleanup deps and package.json

25a4298

fix dedent dependency

02ce02a

hobbes7878 marked this pull request as ready for review July 21, 2023 10:53

hobbes7878 requested a review from JReinhold July 21, 2023 11:06

ndelangen reviewed Jul 25, 2023

View reviewed changes

ndelangen approved these changes Jul 25, 2023

View reviewed changes

ilsubyeega suggested changes Jul 25, 2023

View reviewed changes

src/plugins/vite-svelte-csf.ts Outdated Show resolved Hide resolved

Update src/plugins/vite-svelte-csf.ts

5857238

Co-authored-by: ilsubyeega <ilsubyeega@outlook.com>

ndelangen reviewed Jul 26, 2023

View reviewed changes

adds handling for Windows paths

fa5505b

JReinhold mentioned this pull request Jul 28, 2023

Update dependencies to support svelte@4 #112

Merged

JReinhold self-assigned this Jul 28, 2023

JReinhold added the bug Something isn't working label Jul 28, 2023

JReinhold requested changes Jul 28, 2023

View reviewed changes

hobbes7878 and others added 3 commits July 29, 2023 11:35

Apply suggestions from code review

70c1500

Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>

cleanup scripts and expand vite peer deps range

9abce0f

strict typescript

139f47e

hobbes7878 added 3 commits July 29, 2023 13:47

force resolutions of string-width and wrap-ansi

920391d

fix node-logger calls so just one arg is passed

acf9548

fix a few outstanding type errors in parser and preset

079431e

hobbes7878 requested a review from JReinhold July 29, 2023 14:10

stray comment

529e422

JReinhold added the patch Increment the patch version when merged label Jul 29, 2023

hobbes7878 added 3 commits July 29, 2023 21:26

convert to pnpm

7aad038

regen lockfile

78fc850

add fetch depth for actions/checkout@v3

9cebdba

JReinhold changed the title ~~Update build system~~ Fix generated ESM, revamp build system Jul 31, 2023

JReinhold reviewed Jul 31, 2023

View reviewed changes

cleanup package.json scripts and pnpm in release workflow

4269195

JReinhold approved these changes Jul 31, 2023

View reviewed changes

JReinhold merged commit de6b0f5 into main Jul 31, 2023

shilman added the released This issue/pull request has been released. label Jul 31, 2023

benmccann mentioned this pull request Aug 30, 2023

[Bug] jest-transform missing #132

Closed

j3rem1e deleted the module-build branch September 28, 2023 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix generated ESM, revamp build system #113

Fix generated ESM, revamp build system #113

hobbes7878 commented Jul 19, 2023 •

edited by shilman

Loading

JReinhold commented Jul 20, 2023

hobbes7878 commented Jul 20, 2023

socket-security bot commented Jul 20, 2023 •

edited

Loading

socket-security bot commented Jul 20, 2023 •

edited

Loading

hobbes7878 commented Jul 21, 2023

davemackintosh commented Jul 23, 2023

olafurw commented Jul 24, 2023

ndelangen Jul 25, 2023 •

edited

Loading

hobbes7878 Jul 26, 2023

ndelangen Jul 26, 2023

hobbes7878 Jul 26, 2023

ilsubyeega Jul 26, 2023 •

edited

Loading

JReinhold left a comment •

edited

Loading

hobbes7878 commented Jul 29, 2023

hobbes7878 commented Jul 29, 2023 •

edited

Loading

JReinhold commented Jul 29, 2023

hobbes7878 commented Jul 29, 2023

JReinhold Jul 31, 2023

hobbes7878 Jul 31, 2023

JReinhold commented Jul 31, 2023

shilman commented Jul 31, 2023

oscard0m commented Jul 31, 2023

Fix generated ESM, revamp build system #113

Fix generated ESM, revamp build system #113

Conversation

hobbes7878 commented Jul 19, 2023 • edited by shilman Loading

JReinhold commented Jul 20, 2023

hobbes7878 commented Jul 20, 2023

socket-security bot commented Jul 20, 2023 • edited Loading

socket-security bot commented Jul 20, 2023 • edited Loading

Next steps

hobbes7878 commented Jul 21, 2023

davemackintosh commented Jul 23, 2023

olafurw commented Jul 24, 2023

ndelangen Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

hobbes7878 Jul 26, 2023

Choose a reason for hiding this comment

ndelangen Jul 26, 2023

Choose a reason for hiding this comment

hobbes7878 Jul 26, 2023

Choose a reason for hiding this comment

ilsubyeega Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

JReinhold left a comment • edited Loading

Choose a reason for hiding this comment

hobbes7878 commented Jul 29, 2023

hobbes7878 commented Jul 29, 2023 • edited Loading

JReinhold commented Jul 29, 2023

hobbes7878 commented Jul 29, 2023

JReinhold Jul 31, 2023

Choose a reason for hiding this comment

hobbes7878 Jul 31, 2023

Choose a reason for hiding this comment

JReinhold commented Jul 31, 2023

shilman commented Jul 31, 2023

oscard0m commented Jul 31, 2023

hobbes7878 commented Jul 19, 2023 •

edited by shilman

Loading

socket-security bot commented Jul 20, 2023 •

edited

Loading

socket-security bot commented Jul 20, 2023 •

edited

Loading

ndelangen Jul 25, 2023 •

edited

Loading

ilsubyeega Jul 26, 2023 •

edited

Loading

JReinhold left a comment •

edited

Loading

hobbes7878 commented Jul 29, 2023 •

edited

Loading