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

fix error when used with rollup@1.0.0+ #27

Merged
merged 2 commits into from Jan 13, 2019
Merged

fix error when used with rollup@1.0.0+ #27

merged 2 commits into from Jan 13, 2019

Conversation

mohsinulhaq
Copy link
Contributor

fixes #26

@mohsinulhaq
Copy link
Contributor Author

@TrySound @transitive-bullshit can we please merge this PR?

@mohsinulhaq
Copy link
Contributor Author

@TrySound i hope you are not ignoring development to this repo because no one is contributing to your beerpay 😮

@@ -1,5 +1,9 @@
dist: xenial
Copy link
Owner

Choose a reason for hiding this comment

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

What this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testing on latest LTS ubuntu distro 16.x instead of default 14.x
won't really matter with our use case
but i thought it is a nice thing to have

- "8"
- "6"
cache: yarn
before_install:
yarn config set ignore-engines true
Copy link
Owner

Choose a reason for hiding this comment

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

What this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn throws error when installing a transitive package where engine is marked as requiring node >=8
npm works fine. This command relaxes that check like npm.
https://stackoverflow.com/questions/45088031/how-to-ignore-incompatible-engine-node-error-on-installing-npm-dependencies-wi

Copy link
Owner

Choose a reason for hiding this comment

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

Which package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

language: node_js
node_js:
- "9"
- "10"
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to add also "node" to check in the latest node version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i will add that as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

})
)
.then(
(result): Output => {
Copy link
Owner

Choose a reason for hiding this comment

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

Did prettier formatted this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, via precommit hook. something off?

.map(node => node.end - node.start)
.reduce((acc, size) => acc + size, 0);
.then(({ output }) =>
minify(output.find(chunkOrAsset => chunkOrAsset.code).code, {
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any output without code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there can be assets or code
rollup/rollup#2293

@mohsinulhaq
Copy link
Contributor Author

any other changes needed?

@mohsinulhaq
Copy link
Contributor Author

@TrySound ?

Copy link
Owner

@TrySound TrySound left a comment

Choose a reason for hiding this comment

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

Thanks! Will publish today

@TrySound TrySound merged commit fedb1c8 into TrySound:master Jan 13, 2019
@mohsinulhaq
Copy link
Contributor Author

Thanks!

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.

Usage with rollup 1.0 errors out
2 participants