Closed GiancarlosIO closed 5 years ago
Oh hmm sorry for the trouble. Maybe due to the dependency updates in https://github.com/nytimes/react-tracking/pull/117
Did you get any npm warnings like missing peerDependencies when you upgraded? Does v5.6 work fine for you?
@tizmagik I downgrade to 5.6 and I'm still getting errors, and not, I'm not getting missing peerDependencies with 5.7.
I think it is because I'm using @babel/core@7.4 and core-js@3, corejs 3 don't have the packages es7.symbol.async-iterator, web.dom.iterable', etc. But I don't know why it only fails in react-tracking :thinking: .
Is react-tracking using another way to bundle the code?
Thanks!
Hmm we haven’t changed the way we build the code. We assume any polyfills are done in userland, that’s why the peerDependency on core-js. Happy to look at a repro or something but not sure if this is specific to react-tracking or if it’s just how your polyfills are applied in your app?
@tizmagik I will try to create a repro this weekend :running_man:
FWIW, we have seen the same issue after upgrading to core-js
3.0.0 internally
Thanks @jacekradko -- do you have a repro you could point to, or would you know off-hand what we might be missing on react-tracking side? We haven't changed how we're bundling so I guess it's something to do with how babel runtimes work or something? 🤷♂️
@tizmagik running into this too, will try to put together a repro if i have time. I'd guess that upgrading the dependency in react-tracking, and then publishing a new build will resolve the issues.
Possible (likely) related issue in babel, updates to babel config seem to be the answer: https://github.com/babel/babel/issues/9796
Additional info (migration guide): https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md#babelpreset-env
Reproduction: https://github.com/mckernanin/react-tracking-corejs-v3-repro/tree/master/
New app with CRA, install core-js
+ regenerator-runtime
, import them in index.js like this along with react-tracking
.
Hmm releasing this in v7.0.0 and using it in a couple of apps at NYT caused build failures,
ERROR in ./node_modules/react-tracking/build/withTrackingComponentDecorator.js
Module not found: Error: Can't resolve 'core-js/modules/es.symbol.iterator'
ERROR in ./node_modules/react-tracking/build/withTrackingComponentDecorator.js
Module not found: Error: Can't resolve 'core-js/modules/web.dom-collections.iterator'
Maybe that means we don't have babel configured correctly, OR we're not building react-tracking correctly but either way I think I want to pull this change out of 7.0.0
and release 7.0.1
while we look into a better way to fix the original problem this issue was opened for.
@tizmagik did you upgrade core-js in those projects?
@mckernanin yes, I explicitly listed core-js@3 as a dependency (it was a transitive dependency before that) and it still resulted in build failures. I think the issue is that it's not as simple as upgrading core-js to v3, but that it requires babel configuration updates as well. Maybe we can figure out a better way to build react-tracking so that's not necessary but for now I rather keep the Hooks release separate. My own fault for wanting to lump these two things together 😅
No worries. 7.0.0 works properly with react-scripts
v3.0.0, I just upgraded a production app. Likely a needs something in a custom babel config to work properly. If there's anything I can do to help let me know!
Ah yea you're right, if I update our babel configuration to:
"presets": [
[
"@babel/preset-env",
{
"useBuiltIns": "usage",
+ "corejs": 3
}
],
And ensure that core-js@3
is a direct dependency, then it seems to work.
But, I rather not have folks need to mess with their babel configuration (specifically folks not relying on react-scripts). Maybe we need to move to using microbunlde or something.
Just gave this a spin in our app and adding these options didn't work -- because we're on a lower version of babel 7; the corejs
option was added in @babel@7.4.0.
We could update no problem, but I wonder if it might be better to bundle these requirements up in the build to NPM rather than requiring folks to install additional packages and update config? Will be very hard to coordinate across so many different setups. What about users who don't use Babel? Making react-tracking
easy to use for a few KB could be a bigger win than the alternative.
So if I set core-js@3
as a direct dependency of react-tracking, then it no longer requires changes in user-land babel config. I'm not crazy about that solution though because that means this package would report as +40KB in size (for all of core-js) even though it doesn't actually use all of it: see bundlephobia.
Maybe that's okay for now until we have time to figure out a better way to build/bundle react-tracking?
I published a pre-patch against alpha tag, v7.0.1-0
@damassi mind giving that a spin in your app to see if it solves your build issues?
npm install react-tracking@alpha
Also open to other ideas! 🤗
@tizmagik - works great 👍 As an interim I agree it's probably ok so that consumers don't break / require additional setup, but also that it can be improved upon. Will give a think when I get a sec.
Thanks @damassi -- this update is released as v7.0.1
Closing this issue in favor of: https://github.com/nytimes/react-tracking/issues/127
I'm getting this error after upgrade to v5.7.0
It is the only package that give me that error :thinking: Maybe it is because I'm using core-js v3? :thinking: