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

feat(core): allow non-default-export client config file #1564

Merged
merged 9 commits into from
May 23, 2024
Merged

Conversation

Mister-Hope
Copy link
Member

@Mister-Hope Mister-Hope commented May 20, 2024

do not require export default {} with client config file

useful for:

  • users that only want to add this file for plugin customization purpose
  • plugins which only need to add some styles or library imports on the client side with this

@Mister-Hope Mister-Hope changed the title feat: allow clientConfig file to not have default export feat(core): do not require default export with client config file May 20, 2024
@coveralls
Copy link

coveralls commented May 20, 2024

Pull Request Test Coverage Report for Build 9211065904

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 41.16%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/app/prepare/prepareClientConfigs.ts 0 1 0.0%
Totals Coverage Status
Change from base Build 9184528421: 0.0%
Covered Lines: 688
Relevant Lines: 1709

💛 - Coveralls

Copy link
Member

@meteorlxy meteorlxy left a comment

Choose a reason for hiding this comment

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

  1. Better to check default export in node side instead of filtering it in client code.
  2. Better to add e2e tests for the changes.

@Mister-Hope
Copy link
Member Author

Mister-Hope commented May 21, 2024

I'm not pretty sure about the first one, So you mean importing this file on the node side and see if the default export is available?

So what's the motivation of this? We can't just filter those files which does not have sport because we shall import them anyway to have their library and style file imported. Also, if the file is containing some style files import like css and scss, An error is expected at node side because we are not dealing them with bundlers.

And I think there is no actual performance decrease for using a namespace import, Because only the default export is expected on this file.

@meteorlxy
Copy link
Member

meteorlxy commented May 21, 2024

I mean to check the file content in node side or something. As I remember, v1 did similar thing by checking export default keyword.

It could avoid the extra .map((m) => m.default).filter(Boolean) in client side.

But yes it's nice to have but not such necessary.

@Mister-Hope
Copy link
Member Author

Mister-Hope commented May 21, 2024

I think checking without ast may introduce fail positive, so I prefer the original one. I don't think ast check should be introduced here.

@Mister-Hope
Copy link
Member Author

A e2e test about config file with no export default is added.

@Mister-Hope
Copy link
Member Author

shit, why is windows failing (ー_ー)!!

@@ -0,0 +1 @@
import './test.css'
Copy link
Member

Choose a reason for hiding this comment

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

Should check if the styles are applied correctly to the target elements

@meteorlxy meteorlxy changed the title feat(core): do not require default export with client config file feat(core): allow non-default-export client config file May 23, 2024
@meteorlxy meteorlxy merged commit d3b3cc4 into main May 23, 2024
30 checks passed
@meteorlxy meteorlxy deleted the client-file branch May 23, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants