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.

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

Core: fix PnP compatibility for @storybook/ui and @storybook/router packages #18412

Merged
merged 4 commits into from Jun 10, 2022

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 6, 2022

@Andarist Andarist requested a review from ndelangen Jun 6, 2022
@nx-cloud
Copy link

nx-cloud bot commented Jun 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b77e07b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


Successfully ran 1 target

Sent with 💌 from NxCloud.

@@ -50,6 +50,7 @@
"@storybook/semver": "^7.3.2",
"@storybook/theming": "6.5.0-rc.1",
"core-js": "^3.8.2",
"qs": "^6.10.0",
Copy link
Contributor Author

@Andarist Andarist Jun 6, 2022

Choose a reason for hiding this comment

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

The reported issue was only related to this package

@@ -42,6 +42,7 @@
"dependencies": {
"@storybook/client-logger": "6.5.0-rc.1",
"core-js": "^3.8.2",
"qs": "^6.10.0",
Copy link
Contributor Author

@Andarist Andarist Jun 6, 2022

Choose a reason for hiding this comment

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

but I've audited the whole lib direcotry and found this offender too

@shilman shilman added patch Bugfix & documentation PR that need to be picked to master branch yarn / npm Yarn / npm acting weird labels Jun 6, 2022
@yannbf
Copy link
Member

yannbf commented Jun 6, 2022

That's amazing @Andarist ! I'm updating the branch to reenable the E2E tests that were failing because of this issue, I hope you don't mind!

@yannbf
Copy link
Member

yannbf commented Jun 6, 2022

@Andarist after reenabling the e2e tests, seems like it still fails, however not with the same issue (which is great!)

https://app.circleci.com/pipelines/github/storybookjs/storybook/26404/workflows/a6f23f92-f687-4ae2-8f19-71a8c39964fc/jobs/385085

Any ideas?

@shilman
Copy link
Member

shilman commented Jun 6, 2022

@Andarist it looks like the PnP tests are failing for CRA. Is this something you can look into? If you'd like to pair I'd be happy to look at it together. I'm not sure whether it's related to PnP or just a problem with CRA in the current state of things.

@shilman shilman self-assigned this Jun 6, 2022
@shilman shilman mentioned this pull request Jun 6, 2022
3 tasks
@Andarist
Copy link
Contributor Author

Andarist commented Jun 6, 2022

@yannbf @shilman I think that you are referring to the same problem - correct me if I'm wrong.

So I can easily reproduce this by just calling require('@storybook/react') in node. This is because its main field points to a file within /client/ directory:

"main": "dist/cjs/client/index.js",

this in turn leads to executing this call at the initialization time:
const api = start(renderToDOM, { render });

and that leads to creating a PostmsgTransport and calling this in its constructor:
globalWindow.addEventListener('message', this.handleEvent.bind(this), false);

The problem is that globalWindow is not really a window in node, it's a process object, so it doesn't have an addEventListener method. Note that globalWindow actually is being (sort of) retrieved through an external package but that doesn't change a lot because its main file is super simple:
https://github.com/Raynos/global/blob/91c4362aa2720f7c7a3a085707de7e04f5e77cf3/window.js

Question would be - should node ever execute what is inside that /client/ directory? If not then why did it? Can you provide me with a full script that repro this in a more full project? I've tried to follow your CI setup but it isn't super straightforward and I didn't have enough time right now to analyze it further.

Either way - this doesn't look related to this particular PR and to PnP.

@yannbf
Copy link
Member

yannbf commented Jun 7, 2022

Great point @Andarist! I think I know what's going on and I'll attempt a small change in this PR to check whether the problem will be fixed.

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #18412 (b77e07b) into next (c8a3b7b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             next   #18412   +/-   ##
=======================================
  Coverage   32.04%   32.04%           
=======================================
  Files         975      975           
  Lines       19216    19216           
  Branches     4031     4031           
=======================================
  Hits         6158     6158           
  Misses      12495    12495           
  Partials      563      563           
Impacted Files Coverage Δ
lib/core-common/src/presets.ts 61.66% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8a3b7b...b77e07b. Read the comment docs.

@Andarist
Copy link
Contributor Author

Andarist commented Jun 7, 2022

@yannbf it looks like the e2e and regressions tests are

@shilman shilman assigned ndelangen and unassigned shilman Jun 8, 2022
@yannbf yannbf changed the title Fixed PnP compatibility for bundled ui and router packages Core: fix PnP compatibility for @storybook/ui and @storybook/router packages Jun 10, 2022
@yannbf yannbf merged commit b2f4390 into next Jun 10, 2022
43 of 48 checks passed
@yannbf yannbf deleted the fix/pnp-compat-2 branch Jun 10, 2022
@yannbf
Copy link
Member

yannbf commented Jun 10, 2022

Thanks a lot for your work @Andarist <3

@shilman shilman added the picked Patch/release PRs cherry-picked to master/release branch label Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies patch Bugfix & documentation PR that need to be picked to master branch picked Patch/release PRs cherry-picked to master/release branch yarn / npm Yarn / npm acting weird
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants