-
Notifications
You must be signed in to change notification settings - Fork 364
[types] fix @stylexjs/babel plugin cjs compat #1322
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
base: main
Are you sure you want to change the base?
Conversation
| declare const $$EXPORT_DEFAULT_DECLARATION$$: StyleXTransformObj; | ||
| export default $$EXPORT_DEFAULT_DECLARATION$$; | ||
|
|
||
| declare namespace styleXTransform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid using a namespace here? Other than that the changes look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nmn. I was following this advice from the DefinitelyTyped README. Do you have a recommended solution without a namespace?
When the implementation package uses
module.exports = ..., the DefinitelyTyped package should useexport =, notexport default. (Alternatively, if themodule.exportsis just an object of named properties, the DefinitelyTyped package can use a series of named exports.) The most common obstacle to correcting this problem is confusion about how to export types in addition to the primary export. For example, assume these types are incorrectly usingexport default:export interface Options { // ... } export default function doSomething(options: Options): void;Changing the
export defaultto anexport =creates an error:export interface Options { // ... } declare function doSomething(options: Options): void; export = doSomething; // ^^^^^^^^^^^^^^^^^ // Error: An export assignment cannot be used in a module with other exported elements.To fix this, move the types inside a namespace with the same name as the function:
declare namespace doSomething { export interface Options { // ... } } declare function doSomething(options: doSomething.Options): void; export = doSomething;
| legacyDisableLayers?: boolean; | ||
| }, | ||
| ): string; | ||
| export type StyleXTransformObj = Readonly<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you can use this type and use the export = syntax to make it work with cjs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't seem to find a way to do it with running in to "An export assignment cannot be used in a module with other exported elements.(2309)"
# Conflicts: # packages/typescript-tests/package.json
a948c05 to
2c43f21
Compare
What changed / motivation ?
In ce530d0 I pushed a fix for a TypeScript error that we were seeing.
I'm assuming most other's were not hitting this if they set
skipLibCheck: true. Thev0.16.3release fixed the error and our StyleX integration works as expected.I was reading through old issues I discovered #889 and the attempted fix (#924). I think my commit ultimately reverted their fix and likely caused a regression for some people. See: https://arethetypeswrong.github.io/?p=%40stylexjs%2Fbabel-plugin%400.16.3
My fix in this PR is to go back to using
export =but use a namespace so we don't run into the"An export assignment cannot be used in a module with other exported elements"error. This commit also corrects theRuletype andconfigargument forprocessStylexRules, which have changed since this file was introduced.Sorry for the regression. Thanks!
Linked PR/Issues
#889
Pre-flight checklist
Contribution Guidelines