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

Test for side effects, and remove existing ones #4769

Merged
merged 2 commits into from May 16, 2019

Conversation

@filipesilva
Copy link
Contributor

commented May 10, 2019

Description:
This PR adds tests for side effects to the RxJS ESM bundles.

It also makes use a tslint rule to identify a known side-effect producing pattern, allowing us to refactor that away and remove existing side effects.

Related issue (if exists):
This PR is part of the effort in Angular to remove side effects (angular/angular#29329).

NotificationKind["NEXT"] = "N";
NotificationKind["ERROR"] = "E";
NotificationKind["COMPLETE"] = "C";
})(NotificationKind || (NotificationKind = {}));

This comment has been minimized.

Copy link
@filipesilva

filipesilva May 10, 2019

Author Contributor

This code is retained because of how TS transpiles enums. Newer versions of TS don't suffer from this.

@@ -0,0 +1,622 @@
import { __extends } from "tslib";

This comment has been minimized.

Copy link
@filipesilva

filipesilva May 10, 2019

Author Contributor

This module in particular retains a lot of code.

@@ -0,0 +1,9 @@
This test checks if the side effects for loading Angular packages have changed using <https://github.com/filipesilva/check-side-effects>.

This comment has been minimized.

Copy link
@filipesilva

filipesilva May 10, 2019

Author Contributor

This file explains how to use the side effect test.

@@ -1,584 +1,3 @@
function isFunction(x) {

This comment has been minimized.

Copy link
@filipesilva

filipesilva May 10, 2019

Author Contributor

All this retained code is now gone.

@rxjs-github-bot

This comment has been minimized.

Copy link

commented May 10, 2019

Warnings
⚠️

❗️ Big PR (1)

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

Generated by 🚫 dangerJS

@filipesilva filipesilva force-pushed the filipesilva:side-effects branch 3 times, most recently from 258a055 to 9fe10d3 May 10, 2019
@filipesilva filipesilva force-pushed the filipesilva:side-effects branch from 9fe10d3 to acdb99e May 15, 2019
integration/side-effects/README.md Outdated Show resolved Hide resolved
@benlesh benlesh merged commit e5ab37d into ReactiveX:master May 16, 2019
5 checks passed
5 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: dtslint Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: typescript3 Your tests passed on CircleCI!
Details
@lock lock bot locked as resolved and limited conversation to collaborators Jun 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.