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

check for sideEffects field set to false #5022

Merged
merged 2 commits into from Sep 6, 2019

Conversation

KarishmaGhiya
Copy link
Contributor

@KarishmaGhiya KarishmaGhiya commented Sep 6, 2019

cc : @ramya-rao-a I am not sure if the sideEffects field in core-asynciterator and core-paging were set to true intentionally. Can you ask the appropriate people to review this PR?

Copy link
Contributor

@sophiajt sophiajt left a comment

Choose a reason for hiding this comment

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

We shouldn't change paging and the polyfill module, as these do have side effects. We might be able to limit it to one. We can revisit after the release.

@KarishmaGhiya
Copy link
Contributor Author

@jonathandturner Reverted the changes for core-paging and core-asynciterator

@@ -44,6 +44,7 @@
"unit-test": "npm run unit-test:node && npm run unit-test:browser"
},
"homepage": "https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/core/core-amqp",
"sideEffects": false,
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a setting mostly used by WebPack. Since rollup doesn't use it and has it's own mechanism, doesn't seem like we should be adding this. More context -rollup/rollup#2593
Deferring to @bterlson for any other insights

Copy link
Contributor

Choose a reason for hiding this comment

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

Our customers may be using the package with webpack, which is why it can be useful to have this field.

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is for customer bundling more than our own. Webpack is far and away the most popular tool for this.

@chradek
Copy link
Contributor

chradek commented Sep 6, 2019

As far as I know, core-amqp doesn't contain any side-effects.

Side-note: Do we have a way to test if our package is actually side-effect free? I wonder if creating a bundle of our samples and the package using webpack and running the samples would catch issues from a bad configuration (it's what our customers would likely be doing)

@ramya-rao-a ramya-rao-a requested review from bterlson and removed request for daviwil and ShivangiReja September 6, 2019 02:24
Copy link
Contributor

@southpolesteve southpolesteve left a comment

Choose a reason for hiding this comment

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

Approved for Cosmos. We are side effect free and I kept meaning to add this. Our webpack users will benefit. @chradek I like the idea of having some tool that can help verify this. I am not aware of anything, but maybe @bterlson has already given it some thought.

@bterlson
Copy link
Member

bterlson commented Sep 6, 2019

Confirming the polyfill libs should have sideEffects: true.

amqp-common seems right to me.

I don't know how to scan for side effects. Theoretically speaking it requires an effects tracking type system which would be a fun project but outside the scope of this project 😄 Practically speaking one could probably hack something into Node to track certain classes of side effects (e.g. any writes to types in global scope during module load) but this would likely never catch all side effects and be a lot of work besides.

Testing that the bundles we produce can be composed into working applications using a variety of bundlers seems like the best way to validate this.

@ramya-rao-a ramya-rao-a dismissed sophiajt’s stale review September 6, 2019 17:20

The review comment has been addressed. Dismissing to unblock the PR

@KarishmaGhiya KarishmaGhiya merged commit c004cb9 into Azure:master Sep 6, 2019
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.

None yet

7 participants