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

WITH REPRO STEPS - switchMap not unsubscribing if imported from internal #5641

Closed
radex opened this issue Aug 10, 2020 · 6 comments
Closed

Comments

@radex
Copy link

radex commented Aug 10, 2020

Hi! This is a reopen of #4363, #4230, #3306.

The issue was never solved because reproducible sample was never provided… U N T I L   N O W :))

Here's a repro: https://github.com/radex/rxjs-import-bug-repro

This:

const x = interval(100).pipe(
      switchMap(() => interval(20).pipe(tap(x => console.log(x)))),
      tap(x => console.warn(x)),
    ).subscribe()

    setTimeout(() => {
      console.log('END')
      x.unsubscribe()
    }, 1000)

Naturally should stop logging to console after 1 second. Even if I do imports like this:

import { interval } from 'rxjs/_esm2015/internal/observable/interval'
import { timer } from 'rxjs/_esm2015/internal/observable/timer'
import { take } from 'rxjs/_esm2015/internal/operators/take'
import { switchMap } from 'rxjs/_esm2015/internal/operators/switchMap'
import { takeUntil } from 'rxjs/_esm2015/internal/operators/takeUntil'
import { tap } from 'rxjs/_esm2015/internal/operators/tap'

All is well. However, if I do this:

import { interval } from 'rxjs/observable/interval'
import { timer } from 'rxjs/observable/timer'
import { take, switchMap, takeUntil, tap } from 'rxjs/operators'

and add this to webpack:

const rxPaths = require('rxjs/_esm2015/path-mapping')()

// remove `rxjs-compat`
const fixedRxPaths = {}
for (let k in rxPaths) {
  const value = rxPaths[k]
  const fixed = value.startsWith('rxjs-compat') ? value.replace('rxjs-compat/_esm2015', 'rxjs/_esm2015/internal') : value
  fixedRxPaths[k] = fixed
}

// ....

config = {
...
  resolve: {
    ...
    alias: {
      ...fixedRxPaths
    }
  }
}

Then I can reproduce the problem. Contrived example? Sure. But this is an simplified extraction from a real app. Why would I do such an awful thing and not just import from rxjs as is documented? Because I'm shipping the app both on web and with React Native. And the latter's packager is not smart enough to tree shake unused code. That's why doing code transforms (in webpack and babel) and point to internal so that I can avoid a big chunk of RxJS weight on RN (and keep same code for web)

@radex
Copy link
Author

radex commented Aug 10, 2020

BTW: I realize now that the cause of my problems is that my path rewriting is subtly wrong, and I import from two different copies of rxjs - _esm2015 and _esm5… That doesn't explain why this is broken, though

@kwonoj
Copy link
Member

kwonoj commented Aug 10, 2020

Why would I do such an awful thing and not just import from rxjs as is documented?

As you already described, we have a specific way to consumer import our API surface and strongly prevent to import internal import site. I'm afraid this is not something we can actively trying to resolve. Does code work with documented import which we expose as public interface?

@radex
Copy link
Author

radex commented Aug 10, 2020

Does code work with documented import which we expose as public interface?

(Yes, but) I'm not sure you don't understand my point. The documented import is unsuitable for React Native (Metro bundler), because it results in the entire rxjs being bundled… and it's a very big chunk of code, most of which my app does not need. Hence the desire to be able to import only specific parts of rxjs.

CC @cartant - I'm pinging you because you kept asking on other threads for a reproducible demo. It's in the first comment on this issue.

@kwonoj
Copy link
Member

kwonoj commented Aug 10, 2020

I am aware your motivation but

The documented import is unsuitable for React Native (Metro bundler), because it results in the entire rxjs being bundled

Shouldn't this be raised upstream metro to support tree shaking? Our tree shaking config is pretty much work for majority of bundler, while there is no such thing like ECMA standard for tree shaking it's de-facto across most of bundlers.

  1. Does metro not support tree shaking at all? or particularly rx or specific lib fails?
  2. Does metro upstream not have plan to support tree shaking at all?

I still see this is bundler & your application config issue rather than core failed to export proper API surface to users.

@cartant
Copy link
Collaborator

cartant commented Aug 10, 2020

@radex You've not filled out the issue template, so IDK what version you are using. If you are seeing this with v6, there are several interop changes that were made in v7 that might explain it. Regardless of that, you should be doing whatever's necessary to ensure all of your imports are using the same module.

I can see that there's a link to a repo, but it has a whole bunch of deps that I don't want to vet - before installing locally. Essentially, I'd like to know whether this problem is still effected in the v7 beta.

@kwonoj
Copy link
Member

kwonoj commented Jan 22, 2021

Per comment, so far my understanding this request is very specific to metro bundler and we don't have additional information to rule out this is rxjs specific. If this is still an issue and could be reproducible with 1. without metro 2. following public rxjs exports, please create new issue or discussions.

@kwonoj kwonoj closed this as completed Jan 22, 2021
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

No branches or pull requests

3 participants