Skip to content

Fix react-jss exports from jss-starter-kit #1001

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.

Sign up for GitHub

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

Merged
HenriBeck merged 4 commits into from
Jan 27, 2019
Merged

Fix react-jss exports from jss-starter-kit #1001

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7f89e1
Fix react-jss exports from jss-starter-kit
Jan 26, 2019
f9b2720
Update changelog and update size snapshot
Jan 26, 2019
b671d61
Run prettier
Jan 26, 2019
91414e8
Add some missing exports
Jan 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## Next

### Bug fixes

- [jss-starter-kit] Fix react-jss exports and add missing jss exports ([#1001](https://github.com/cssinjs/jss/pull/1001))

## 10.0.0-alpha.9 (2019-1-25)

### Bug fixes
Expand Down
24 changes: 12 additions & 12 deletions packages/jss-starter-kit/.size-snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"dist/jss-starter-kit.js": {
"bundled": 81048,
"minified": 32915,
"gzipped": 9378
"bundled": 81472,
"minified": 33168,
"gzipped": 9474
},
"dist/jss-starter-kit.min.js": {
"bundled": 80294,
"minified": 32437,
"gzipped": 9166
"bundled": 80718,
"minified": 32710,
"gzipped": 9262
},
"dist/jss-starter-kit.cjs.js": {
"bundled": 2341,
"minified": 2127,
"gzipped": 689
"bundled": 2762,
"minified": 2513,
"gzipped": 795
},
"dist/jss-starter-kit.esm.js": {
"bundled": 1385,
"minified": 1273,
"gzipped": 462,
"bundled": 1479,
"minified": 1362,
"gzipped": 517,
"treeshaked": {
"rollup": {
"code": 792,
Expand Down
11 changes: 10 additions & 1 deletion packages/jss-starter-kit/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,16 @@ jss.setup(preset())

export {jss, preset}

export * as reactJss from 'react-jss'
export {SheetsRegistry, SheetsManager, createGenerateId} from 'jss'

export {
Copy link
Copy Markdown
Member

@kof kof Jan 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, that was supposed to be exported as exports.reactJss.withTheme , so that its in reactJss namespace

Copy link
Copy Markdown
Member Author

@HenriBeck HenriBeck Jan 26, 2019

Choose a reason for hiding this comment

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

But then you couldn’t import them directly which isn’t user-friendly in my opinion

Copy link
Copy Markdown
Member

@kof kof Jan 27, 2019

Choose a reason for hiding this comment

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

Its not too user friendly, but on the other hand, those exports might have potential collision now and because we need to maintain the named exports here ass well.

Copy link
Copy Markdown
Member

@kof kof Jan 27, 2019

Choose a reason for hiding this comment

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

What if we export everything under one namespace but in a dynamic way? without maintaining each export name manually?

Copy link
Copy Markdown
Member Author

@HenriBeck HenriBeck Jan 27, 2019

Choose a reason for hiding this comment

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

I don't think there is a way to have every member exported right now. Rollup doesn't bundle something like export * from 'react-jss correctly I believe.

@TrySound do we need to enable something to support this syntax?

Copy link
Copy Markdown
Member

@TrySound TrySound Jan 27, 2019

Choose a reason for hiding this comment

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

Rollup is able yo handle export * syntax just fine. It expands it into named imports internally.

Export * as ns is not supported since its not a part of spec yet.

Copy link
Copy Markdown
Member

@TrySound TrySound Jan 27, 2019

Choose a reason for hiding this comment

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

Ah, I see. It does not expand this because it doesnt know about external dependency. Who resolves this incorrectly? Webpack?

Copy link
Copy Markdown
Member Author

@HenriBeck HenriBeck Jan 27, 2019

Choose a reason for hiding this comment

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

We aren't using webpack anymore, at least not for building.

Copy link
Copy Markdown
Member

@TrySound TrySound Jan 27, 2019

Choose a reason for hiding this comment

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

So what is the problem? Rollup will show collisions if you will bundle react-jss dep in umd.

Copy link
Copy Markdown
Member Author

@HenriBeck HenriBeck Jan 27, 2019

Choose a reason for hiding this comment

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

Figured it out. The problem was that react-jss wasn't declared a dependency of jss-starter-kit.

withTheme,
createTheming,
default as withStyles,
ThemeProvider,
JssProvider
} from 'react-jss'

export {default as functions} from 'jss-plugin-rule-value-function'
export {default as observable} from 'jss-plugin-rule-value-observable'
export {default as template} from 'jss-plugin-template'
Expand Down
⚡