Skip to content

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

Merged
HenriBeck merged 4 commits intomasterfrom
jss-starter-kit/fix-react-jss-export
Jan 27, 2019
Merged

Fix react-jss exports from jss-starter-kit#1001
HenriBeck merged 4 commits intomasterfrom
jss-starter-kit/fix-react-jss-export

Conversation

@HenriBeck
Copy link
Copy Markdown
Member

What would you like to add/fix?
Fix react-jss export and add some missing exports from jss

Corresponding issue (if exists): #948

@HenriBeck HenriBeck requested a review from kof January 26, 2019 16:56
Comment thread packages/jss-starter-kit/src/index.js Outdated
export * as reactJss from 'react-jss'
export {SheetsRegistry, SheetsManager, createGenerateId} from 'jss'

export {
Copy link
Copy Markdown
Member

@kof kof Jan 26, 2019

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

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

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

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

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

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

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

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

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

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.

@HenriBeck HenriBeck requested review from TrySound and kof January 27, 2019 17:18
@HenriBeck
Copy link
Copy Markdown
Member Author

I don't think we should export react-jss under reactJss as this will make it complicated to import and not easily switchable to production code.

@HenriBeck HenriBeck merged commit 92013e2 into master Jan 27, 2019
@HenriBeck HenriBeck deleted the jss-starter-kit/fix-react-jss-export branch January 27, 2019 17:34
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