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

CLI: add warning when main config does not use default exports #20802

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Jan 26, 2023

Telescoping off of #20797

What I did

This PR does a couple of things:

  1. Introduces a new piece of metadata in ConfigFile from csf-tools, where whenever you use readConfig, you will be able to check whether a main.js file contains a default export or not by using mainConfig.hasDefaultExport
  2. Adds a warning to support enforcing the "default exports" format in main.js. It happens in a core piece of code which gets executed in build-dev, build-static and telemetry. It warns users only once.
    Please refer to CLI: generate main config with default exports #20797 for more context.

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@yannbf yannbf added feature request core csf ci:daily Run the CI jobs that normally run in the daily job. labels Jan 26, 2023
@IanVS
Copy link
Member

IanVS commented Jan 26, 2023

I guess we should remove this suggestion from the vite builder readme, then.

Or alternatively, you can use named exports:

@yannbf
Copy link
Member Author

yannbf commented Jan 27, 2023

I guess we should remove this suggestion from the vite builder readme, then.

Or alternatively, you can use named exports:

Thanks! It's addressed here #20801

@yannbf yannbf force-pushed the feat/default-exports-in-main branch from b6d7fcc to 07bb9b9 Compare January 30, 2023 08:35
@yannbf yannbf force-pushed the feat/warn-regarding-default-exports-in-main branch from 1b1db9f to 8f09b18 Compare January 30, 2023 08:35
@socket-security
Copy link

socket-security bot commented Jan 30, 2023

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected malware ✅ 0 issues
Bot Commands

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@2.4.2

Powered by socket.dev

Base automatically changed from feat/default-exports-in-main to next January 31, 2023 07:48
- involves changes in csf-tools to define a flag that tells us there is (or not) a default export
- involves changes in affected packages now that loadMainConfig is async
@yannbf yannbf force-pushed the feat/warn-regarding-default-exports-in-main branch from 051d4ee to 95a3b53 Compare January 31, 2023 09:24
@yannbf yannbf force-pushed the feat/warn-regarding-default-exports-in-main branch from 95a3b53 to 55ebe5b Compare January 31, 2023 09:36
@yannbf yannbf merged commit 56e446a into next Jan 31, 2023
@yannbf yannbf deleted the feat/warn-regarding-default-exports-in-main branch January 31, 2023 12:19
@stevezhu
Copy link

stevezhu commented Feb 2, 2023

@yannbf glob is a peer dep of glob-promise. Should probably be reinstalled.
https://github.com/ahmadnassri/node-glob-promise/blob/v6.0.2/package.json#L40-L42

 WARN  Issues with peer dependencies found
apps/projection
└─┬ @storybook/addon-essentials 7.0.0-beta.39
  └─┬ @storybook/addon-controls 7.0.0-beta.39
    └─┬ @storybook/blocks 7.0.0-beta.39
      └─┬ @storybook/docs-tools 7.0.0-beta.39
        └─┬ @storybook/core-common 7.0.0-beta.39
          └─┬ glob-promise 4.2.2
            └── ✕ missing peer glob@^7.1.6
Peer dependencies that should be installed:
  glob@^7.1.6

But also is it really necessary to install an entire wrapper package just to make glob return a promise? It's just a 5 line promise wrapper.

https://github.com/ahmadnassri/node-glob-promise/blob/master/lib/index.js

@ndelangen
Copy link
Member

I'll fix this today, thank you for bringing this to our attention @stevezhu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. core csf feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants