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

Upgrade all esm ready packages #1029

Merged
merged 6 commits into from
Feb 12, 2019
Merged

Upgrade all esm ready packages #1029

merged 6 commits into from
Feb 12, 2019

Conversation

TrySound
Copy link
Member

In this diff I upgraded

  • hyphenate-style-name
  • css-initials
  • css-vendor

which were converted to esm.

This means, that all jss and all plugins may be consumed by unpkg with
module flag like https://unpkg.com/jss-preset-default?module

We probably may get rid from unpkg prop in favour of ?module flag for
non react packages.

@TrySound TrySound requested review from kof and HenriBeck February 10, 2019 20:53
rollup.config.js Outdated
@@ -86,7 +88,7 @@ export default [
plugins: [
nodeResolve(),
babel(getBabelOptions({useESModules: true})),
commonjs(commonjsOptions),
reactPkg && commonjs(commonjsOptions),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

To make sure we will never ever add another commonjs package in our dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

but why this conditional and not for everything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we dropped commonjs for everything except react based packages.

Copy link
Member

Choose a reason for hiding this comment

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

it just seems like we are addng there another list to maintain

Copy link
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 it will be changed before react will accept esm.

@kof
Copy link
Member

kof commented Feb 11, 2019

@AleshaOleg is looking into css-vendor problem

yarn.lock Outdated
@@ -5,21 +5,18 @@
"@babel/code-frame@^7.0.0":
version "7.0.0"
resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0.tgz#06e2ab19bdb535385559aabb5ba59729482800f8"
integrity sha512-OfC2uemaknXr87bdLUkWog7nYuliM9Ij5HUcajsVcMCpQrcLmtxRbVFTIqmcSkSeYRBFBRxs2FiUqFJDLdiebA==
Copy link
Member Author

Choose a reason for hiding this comment

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

@kof Update your yarn please

Copy link
Member

Choose a reason for hiding this comment

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

damn, what happened here? we have 1.9 in engines

Copy link
Member

Choose a reason for hiding this comment

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

should we have 1.14?

Copy link
Member

Choose a reason for hiding this comment

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

no 1.13, 1.14 is rc

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have 1.13. integrity hash was added after 1.9

Copy link
Member

Choose a reason for hiding this comment

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

strange, upgraded, but it doesn't add themm back

Copy link
Member Author

Choose a reason for hiding this comment

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

Revert yarn.lock and run yarn again

@AleshaOleg
Copy link
Member

Tests are passed, but red cross still there. Who knows what's wrong?

@kof
Copy link
Member

kof commented Feb 11, 2019

there is another issue https://travis-ci.org/cssinjs/jss/builds/491780740#L612 with flow, it can't find react-jss dependency, I have the same locally

@AleshaOleg
Copy link
Member

Ah, ok. I had it too today. I did yarn run build before running tests, and then this error went away.

@kof
Copy link
Member

kof commented Feb 11, 2019

for me run build doesn't fix this, neither it does for the ci

@HenriBeck
Copy link
Member

Will also take a look at the issue

@@ -48,6 +48,11 @@ const getBabelOptions = ({useESModules}) => ({
})

const commonjsOptions = {
include: [
/\/node_modules\/react\//,
Copy link
Member

Choose a reason for hiding this comment

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

This here creates the issue with flow.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I get it. We use ancient rollup version which silently swallowed commonjs errors without emitting files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Upgrading it in separate branch

Copy link
Member

Choose a reason for hiding this comment

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

So upgrading to rollup v1 would solve the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will show the real error which is swallowed now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like the error was in missed react-display-name package which is a transitive dependency of theming.

TrySound and others added 6 commits February 12, 2019 00:36
In this diff I upgraded

- hyphenate-style-name
- css-initials
- css-vendor

which were converted to esm.

This means, that all jss and all plugins may be consumed by unpkg with
module flag like https://unpkg.com/jss-preset-default?module

We probably may get rid from unpkg prop in favour of `?module` flag for
non react packages.
@HenriBeck
Copy link
Member

Looks good now 👍

@kof kof merged commit 5cbd40b into master Feb 12, 2019
@TrySound TrySound deleted the full-esm-support branch February 12, 2019 11:27
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019
* Upgrade all esm ready packages

In this diff I upgraded

- hyphenate-style-name
- css-initials
- css-vendor

which were converted to esm.

This means, that all jss and all plugins may be consumed by unpkg with
module flag like https://unpkg.com/jss-preset-default?module

We probably may get rid from unpkg prop in favour of `?module` flag for
non react packages.

* Replace react pkgs list with commonjs packages list

* upgrade css-vendor

* Revert "upgrade css-vendor"

This reverts commit ec623f7.

* upgrade css-vendor

* set yarn 1.13 in engines
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.

4 participants