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

Add export default option #66

Closed
wants to merge 3 commits into from
Closed

Add export default option #66

wants to merge 3 commits into from

Conversation

jsDotCr
Copy link

@jsDotCr jsDotCr commented Apr 5, 2019

Hi there! I'm using this package and it all worked fine until the CI started complaining about:

ERROR: /home/travis/build/.../src/....css.d.ts: `export =` is not supported by @babel/plugin-transform-typescript
Please consider using `export <value>;`.
  5 |   readonly ellipsis: string
  6 | }
> 7 | export = styles
    | ^
  8 | 

Yup, we are using babel to transpile. It does not support the export assignment syntax which exist for backward compatibility with earlier versions of TypeScript (they probably refer to v.1.5, when they changed their understanding of modules).

I didn't want to introduce any breaking change, so I added an extra option to opt in for the default export declaration (that is the main workaround suggested by the babel folks; and it seems to be working fine) rather than the export assignment.

Let me know if you have any comment or feedbacks, happy to help and clarify!

@henrinormak
Copy link

henrinormak commented May 16, 2019

@Quramy - Would be nice to merge this, otherwise the package is pretty much not usable (have to revert back to an older version).

@@ -133,6 +160,7 @@ You can set the following options:
* `option.outDir`: Output directory(default: `option.searchDir`).
* `option.camelCase`: Camelize CSS class tokens.
* `option.EOL`: EOL (end of line) for the generated `d.ts` files. Possible values `'\n'` or `'\r\n'`(default: `os.EOL`).
* `option.exportDefault`: Uses `export default styles` syntax fir the generated `d.ts` files.

Choose a reason for hiding this comment

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

*for

@rfgamaral
Copy link

I didn't want to introduce any breaking change, so I added an extra option to opt in for the default export declaration (that is the main workaround suggested by the babel folks; and it seems to be working fine) rather than the export assignment.

Would it really be a breaking change if you forced a default export declaration (without the option) or even setting the option to true by default? I mean, TypeScript will handle and work with both forms, for TypeScript this will be transparent, for everyone else, it will start working properly (so it's a bug fix, of sorts). Or am I wrong?

@rfgamaral
Copy link

I didn't want to introduce any breaking change, so I added an extra option to opt in for the default export declaration (that is the main workaround suggested by the babel folks; and it seems to be working fine) rather than the export assignment.

Would it really be a breaking change if you forced a default export declaration (without the option) or even setting the option to true by default? I mean, TypeScript will handle and work with both forms, for TypeScript this will be transparent, for everyone else, it will start working properly (so it's a bug fix, of sorts). Or am I wrong?

Thinking about this and it looks to me that this project is not quite using semver yet (some breaking changes have been introduced in the past and the major version is still 0). As such and to simplify things, I would remove the exportDefault option altogether and force that as the default behavior. But not without changing styles to <basename> of the .css file, like #27 demonstrates. This would also help with #63.

What does everyone think?

@renanvalentin
Copy link

Can we merge this PR?

@blacklane blacklane closed this by deleting the head repository Mar 13, 2023
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.

5 participants