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

rewrite console.log to console.warn to show that main field isn't used #1

Merged
merged 1 commit into from
Feb 22, 2018

Conversation

cilice
Copy link
Contributor

@cilice cilice commented Feb 21, 2018

Hey!

I've read ReactiveX/rxjs#3227 (comment) and wanted to show you that webpack doesn't de-opt to using the main field, it continues to use the module, it's just that tree-shaking doesn't work because it'll de-optimize an esm build to a cjs build and cannot tree-shake it properly. (This might also maybe solvable if once webpack 4 lands and "sideEffects": false becomes more common in package.json)

That this PR does is just simply rewriting console.log to console.error in the dist folder, so UMD builds are always going to have console.error.

In any cases I've tried, webpack never went for the UMD build and always picked the ESM build.

@jayphelps
Copy link
Owner

jayphelps commented Feb 21, 2018

webpack doesn't de-opt to using the main field, it continues to use the module, it's just that tree-shaking doesn't work because it'll de-optimize an esm build to a cjs build and cannot tree-shake it properly

Shoot I'm still not following what you mean. It sounds like you're saying the same thing I was? Webpack won't use the ESM if another dependency also depends on that package but doesn't support ESM because it would be unsafe to have two copies of the code included.

Thanks for bearing with me if I indeed I'm missing something!

@cilice
Copy link
Contributor Author

cilice commented Feb 22, 2018

It still uses the ESM build if the dependency is using the CJS build, but what actually happens is that ESM is being emulated to be CJS because it's downwards compatible (or webpack pretends it to be)

With this PR, ESM test-1 has console.log and UMD build of test-1 has console.error. If you include test-2 in the app build, it still has console.log in it (from ESM) and not the console.error from UMD.

@jayphelps jayphelps merged commit 049def8 into jayphelps:master Feb 22, 2018
@jayphelps
Copy link
Owner

ah ha! That makes sense and I'm seeing that now too. Thanks for being patient and bringing this up!

Are you aware of any side effects of this as far as it being the solution to ReactiveX/rxjs#3227 ?

@cilice
Copy link
Contributor Author

cilice commented Feb 22, 2018

You are welcome, I’m glad really glad that I could help somehow.

And no, I’m not aware of any side effects, but I think even if rxjs doesn’t switch to a flat bundle, the whole problem wouldn’t be there if they still would provide the alias. Because it’s always the same entry, and if webpack picks ESM, the alias rewrites would work in the webpack config.

So the problem of double imports of ESM and CJS and Observable1 instanceof Observable2 errors would never occur, when “rxjs/operators” is a CJS file and webpack has the alias paths.

It’s either option:

  1. Everything is imported from “rxjs”, no deeper links, webpack picks ESM or CJS
  2. You keep “rxjs/operators” and have an alias rewrite if you use ESM. it’s CJS by default and you overwrite it to ESM with the alias.

At least it’s those two that I see now :D

@jayphelps
Copy link
Owner

@cilice yep those two choices are where I've landed currently as well. 🍻

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.

2 participants