Interaction tracking follow up #13509
Conversation
scripts/rollup/build.js
Outdated
|
|
||
| pureExternalModules.forEach(module => { | ||
| const regExp = new RegExp( | ||
| `(?<!= *)require\\(["']${module}["']\\)[,;]`, |
There was a problem hiding this comment.
What does the first part of this do?
There was a problem hiding this comment.
It's a negative look behind to make sure the required module isn't being assigned to a variable.
There was a problem hiding this comment.
To be clear, this is definitely a hack.
No. I'm doing what the docs say to do wrt We can't update the Rollup version to see if it's fixed without addressing circular dependency issues between |
As best I can tell, it just looks like a bug. We're using Rollup 0.52.1 but the latest version is 0.65.0. I don't think we can upgrade past 0.53 or 0.54 (I forget) without hitting the circular dependency issue, and that small upgrade doesn't fix the bug. |
|
React: size: 🔺+37.3%, gzip: 🔺+34.1% ReactDOM: size: -2.1%, gzip: -2.6% Details of bundled changes.Comparing: 0452c9b...bc6f98f react
react-dom
react-art
react-test-renderer
react-scheduler
react-native-renderer
Generated by 🚫 dangerJS |
It would still be good to figure out the exact cause if it doesn't take more than 10 minutes. For example by removing all code in ReactDOM except the import. Seeing if the result still includes the import. If it still reproes, you could create a small repro for rollup. |
|
I already spent well over 10 minutes trying to track down the exact cause 😄 I mainly want to unblock a sync for now. Seems like we can file a repro with Rollup later? |
|
Sure. It's just that if we don't have the incentive we usually don't do that for many months. Since this introduces some complexity and fragility a self-imposed incentive could be helpful. |
There was a problem hiding this comment.
Might be unrelated to this PR, but I think it's in scope of this follow-up.
It appears that if I build react-dom, CJS bundles include require('interaction-tracking'). Regardless of whether we strip it out with this PR, it would still be there for DEV and PROFILE bundles. But react-dom doesn't have interaction-tracking in dependencies. This means it wouldn't be installed and would fail, unless I'm missing something.
|
That's a good catch Dan. I have not yet added deps for interaction-tracking to |
|
I scanned all build modules and confirmed that only the following files reference the
I've updated the corresponding My gut tells me that the best compromise to address deduplication concerns and non-backwards-breaking release for Since this is a new, relatively unproven package– if we release it with a > 1.0 version, I should probably also rename the exported methods with an |
|
Can't you just skip circular dependency warnings in |
|
It's not a warning. It's a runtime error. |
|
Dan and I chatted more about this out of band and realized that a new interaction-tracking dependency would also be problematic for UMD builds, because it would be a backwards breaking change for existing apps (for example, existing code pens). So I think the best plan forward is this:
|
bvaughn
force-pushed
the
interaction-tracking-follow-up
branch
from
807cdb1 to
17d0f06
Compare
|
I just pushed the React re-export change. An added benefit was that I was able to revert the Rollup strip-unused-require hack. |
|
We already had test cases for the interaction-tracking package when the feature flag was enabled and disabled, but I went ahead and added another test that would cover the built bundle– just to ensure that the functionality that was common across all builds (e.g. tracked functions are run, wrapped functions are run, etc.) works. |
|
Yay! Packaging fixture is happy now too. |
|
Why do we have to reexport it on CJS builds? I don’t care about UMD builds because they’re a shit show regardless. But let’s minimize the damage. |
|
I think this should go on the secrets object and then we build a new script tag for interaction tracking that only re-exports the api from the React bundle. There should be no public api for this on React even if it is temporarily packaged in it. That’s how we did it with React and ReactDOM back in the day. Everything was actually in React bundle. Seems like that strategy worked well |
|
In CJS we have another concern: versioning. If you have two versions of This would only help if we encourage React users to consume tracking from the React object of course. |
|
The goal of this package, just like the react-scheduler is to make it universal and used outside React and hopefully eventually part of the platform. It seems to me that both of these new packages need to become peer dependencies for that to happen. |
|
Since deferredUpdates is gone, the async strategy requires access to the scheduler. Seems like we imminently need a solution that brings it out of React too. |
Yeah. But that would be a breaking change for
It can still be used universally by itself if React re-exports it. But it's a migration path to that. I don't know how you plan to make it a peer now because
|
|
Was driving home from work so I'm just now seeing this discussion, but I agree with Dan's recent comments. Don't really have much to add. I'm not thrilled about this, but it seemed like the best compromise given the constraints of not breaking backwards compat in a minor release. |
|
Actually I guess I can respond to this:
We need to access it in a consistent way from renderers. Accessing it by importing e.g. "interaction-tracking" then it would break the UMD build unless we somehow re-mapped such imports to Maybe this would be possible with our Rollup forks system and some sort of shim. I could investigate it tomorrow if you feel it's worth looking into. This would result in kind of a weird combination of |
The secrets thing seems better but let's first figure out the CJS thing.
My concern is that it seems like we're trying to force this in as an intermediate step but we don't have a viable plan for how this is supposed to work in the future. So I don't understand if this intermediate step even makes that upgrade path viable. Why does it have to be What exactly is the breakage if If we can't solve this, this might be a good reason to just publish this as 17. The reason we don't release majors often is because of the breaking changes inside of them are a pain to upgrade. Not because releasing majors by its own is a bad thing. |
|
I’ve never thought about making something both a dep and a peer. That could work. It’s still going to generate backlash though. React is already one too many packages. Every tutorial saying Maybe I’d be more open to this with a different name than |
React UMD bundles inlines the new react-scheduler package and exposes the API via the SECRET_INTERNALS object. UMD bundles of e.g. ReactDOM access the API this way. This avoids breaking backwards compat for UMD bundles (since it avoids requiring a new <script> tag). react CJS bundles to not inline the react-scheduler package. Instead, react-dom and other renderers import the shared NPM module. A hard NPM depedency has been added to react-dom and other renderers on the new react-scheduler package so that upgrades will not require any additional npm-install steps. A UMD bundle of the new react-scheduler also exists which defines the same public API but lazily forwards method calls to the React SECRET_INTERNALS inlined version. This allows the new API to be used in e.g. Code Pens if people want to.
bvaughn
force-pushed
the
interaction-tracking-follow-up
branch
from
c8d5899 to
c5b4db2
Compare
If the concern is in implementation complexity why not keep them as separate files but re-export from both in a single entry point? Conceptually they’re related. |
React production bundle did get a little bigger (+24KB). Looking at a diff, it seems like it's caused by a new invariant related to |
|
My latest commit was just a change to an inline comment, so it shouldn't fail CI. 😄 Looks like a Yarn registry issue:
|
I am not opposed to that. 😄 |
|
Wanna unify then? Seems like this would simplify the bundling setup and UMDs. |
I'm trying this locally now. If it's not too churny, I'll push it here. Else I might do it via a follow up. |
|
As things are currently written, this would have a bit of an impact. During initialization, the subscriptions module registers itself as a subscriber with the tracking module's I think this is avoidable though by just delaying registration until the first time a subscriber is added. I'll make this change and add a test. |
…iptions entry points
|
Looks like it turned out to just simplify a bunch of small things 😄 So done~ |
|
Local testing/fixtures look good. My test app seems happy. Hand inspected the bundles and they look good. Assuming CI is happy– I'm going to land this and we can iterate on it more on Monday if anything else pops up. 😄 I'm feeling good about these changes though! |
|
CI failure is due to Yarn registry again. Restarting... |
| } | ||
|
|
||
| return Object.freeze({ | ||
| __getInteractionsRef: __getInteractionsRef, |
There was a problem hiding this comment.
It would be nice to have some automatic check that verifies the list of exports matches the list of commonjs exports 1:1
There was a problem hiding this comment.
(I understand you added a fixture but someone could add a method to CJS API, and forget to test it in fixture)
There was a problem hiding this comment.
Yeah, that's reasonable. Although I think the bundled UMD solution is pretty reasonable– and it's really only for edge-case usage anyway– I'm not very happy with these "shims" and the way they kind of bypass our normal build system.
I'm not sure how to add an automated check without increasing the hackiness. Any ideas?
There was a problem hiding this comment.
A node script that imports all bundles after build and verifies their exports match up.
Or a Jest test if you can figure out a way to import the entry point shim.
There was a problem hiding this comment.
Whoops, this comment just came through. I'll tackle this with a follow up :)
There was a problem hiding this comment.
Huh. That turned out to be simpler than I thought. 😄
|
I've run all of the CI scripts/checks locally, so I'm going to go ahead and land this without waiting for the Yarn registry issue to be resolved. |
* Merged interaction-tracking package into react-scheduler * Add tracking API to FB+www builds * Added Rollup plugin to strip no-side-effect imports from Rollup bundles * Re-bundle tracking and scheduling APIs on SECRET_INTERNALS object for UMD build (and provide lazy forwarding methods) * Added some additional tests and fixtures * Fixed broken UMD fixture in master (facebook#13512)
* Merged interaction-tracking package into react-scheduler * Add tracking API to FB+www builds * Added Rollup plugin to strip no-side-effect imports from Rollup bundles * Re-bundle tracking and scheduling APIs on SECRET_INTERNALS object for UMD build (and provide lazy forwarding methods) * Added some additional tests and fixtures * Fixed broken UMD fixture in master (#13512)
This was added as part of the "Interaction Tracking" work in facebook#13509 back in 2018. That feature was removed in facebook#20037 in 2020, and this plugin appears to no longer have any effect on the build output.
This was added as part of the "Interaction Tracking" work in facebook#13509 back in 2018. That feature was removed in facebook#20037 in 2020, and this plugin appears to no longer have any effect on the build output.
This was added as part of the "Interaction Tracking" work in facebook#13509 back in 2018. That feature was removed in facebook#20037 in 2020, and this plugin appears to no longer have any effect on the build output.
This was added as part of the "Interaction Tracking" work in facebook#13509 back in 2018. That feature was removed in facebook#20037 in 2020, and this plugin appears to no longer have any effect on the build output.
This was added as part of the "Interaction Tracking" work in facebook#13509 back in 2018. That feature was removed in facebook#20037 in 2020, and this plugin appears to no longer have any effect on the build output.
Follow up for PR #13253. Resolves #13512
React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIREDso that renderers can safely use the new API without requiring any new<script>tags to be added. A UMD fork has been added to redirect react-scheduler imports toReact.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIREDinstead.dependencieson react-scheduler to each reconciler renderer (e.g. react-dom, react-native, react-test-renderer) so that CJS updates will just work without requiring any additionalnpm installsteps.React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIREDexport. Lazy forwards are important so that the<script>tags can be declared in the future-safe order (ie. react-scheduler before react).All methods for the new react-scheduler package (including interaction tracking APIs) have been prefixed with "unstable_" since they're new and relatively unproven.
As part of this change, the react-scheduler code (which used to be inlined into individual renderers) has been moved to the react package. This makes it look like the react UMD bundle has grown more than it has (rather, there is a corresponding decrease in e.g. react-dom). Here are some diffs that show the various CJS and UMD changes (NOTE these diffs reflect suspense being enabled so they may be a bit larger than otherwise expected.)
I've added a new Rollup plugin (
scripts/rollup/plugins/strip-unused-imports.js) to strip unused imports from production bundles so that production react-dom won't have any references to the tracking API.I've also added a new fixture (
fixtures/tracking/index.htmlandfixtures/tracking/script.js) to ensure that the UMD bundles are wired up correctly.Non-blocking follow up work
package.jsons. (I assume 16.5 but for now I've left as 0.1).react-scheduler/trackingandreact-scheduler/tracking-subscriptionsbut I'm not married to these.)