Replies: 44 comments
-
Thanks for proposing this, @thepassle! Totally support this, let's improve the way we import (and export, if talking about our ecosystem packages) things. |
Beta Was this translation helpful? Give feedback.
-
Here's the PR that removes imports to a barrel file by an internally used module #1987 |
Beta Was this translation helpful? Give feedback.
-
Fixed by #1987. 👏 |
Beta Was this translation helpful? Give feedback.
-
Sorry, that PR just addresses one of the problems with barrel files; internal modules importing from a barrel file. The issue itself still exists: the barrel file. Importing |
Beta Was this translation helpful? Give feedback.
-
@thepassle, do you have the internal build entrypoints in mind (i.e. |
Beta Was this translation helpful? Give feedback.
-
The latter; the way MSW exposes exports to users. E.g.: currently if you import Removing the barrel file entirely would be a breaking change, which I can understand is undesirable, especially having just done a big breaking change (although changes to imports often can easily be codemodded for users). However, what we can do is already add separate/grouped/more granular entrypoints so people can import only what they use and need, and keep the barrel file in place for compatibility and avoiding a breaking change. And then on a next major version we could decide if we want to drop the barrel file entirely (which arguably, I think would be better for everyone because it would improve perf all across the board, not just in the browser), but thats something i'll leave up to you :) |
Beta Was this translation helpful? Give feedback.
-
@thepassle what tool did you use to visualize the imports ? |
Beta Was this translation helpful? Give feedback.
-
Madge |
Beta Was this translation helpful? Give feedback.
-
@thepassle, I will give that article a proper read. For now, sharing my initial thoughts. While imports like import { http } from 'msw/http'
import { graphql } from 'msw/graphql'
import { setupWorker } from 'msw/worker' This is a lot of typing to achieve a simple thing. I was quite against The benefit of My second point is that MSW is a devtool. While nobody wants 1MB of JavaScript being pulled by a random third-party tool, you have to consider the implications of it. 1MB on your local machine that gets fetched almost instantly and 1MB in production are two quite different things. I see little risk of large file sizes for developer tools. This is one thing we can openly benefit from, unlike third parties that also ship to production and have to constantly chase smaller build footprints. I most certainly wish to burden the end consumer as little as possible but if it's at a cost of developer experience, I'd say it's not worth it. |
Beta Was this translation helpful? Give feedback.
-
Fundamentally, this has the same problem as literally any other tooling: import { a } from 'react-query' Most packages are structured as a barrel file. So I pull everything now from |
Beta Was this translation helpful? Give feedback.
-
Depending on build tool/environment, Browser ESM is file-by-file imports of every file imported. They can't parse something they haven't imported, so treeshaking can't work in a browser environment (like a compiler-less solution) Generally these can add up, but we can probably get most value by limiting barrel files. Internally, only keep 1, |
Beta Was this translation helpful? Give feedback.
-
While this is current state of the world, package exports are quite young, and I would hope more packages move away from them over time (although this is probably an idealistic assumption). |
Beta Was this translation helpful? Give feedback.
-
Hm, I feel this is subjective. I personally wouldn't mind these imports, especially if it means it means my development/ci will be faster.
I don't use a compiler, I develop in the browser
People can use compilers/transpilers to transpile their source code, but do they do this during local development and in unit tests? And do they also compile their unit test code itself, where they import/setup MSW? Because if not, they're loading a lot of unused, un-treeshaken modules. I know many test runners (one example of where MSW is typically used), like jest, karma, WTR, and more, where test code is not bundled, and not treeshaken.
I'm not arguing this for production code, but they're much less common place in development tooling, like test runners.
I guess we see things differently then, for me anything that slows local development down and can be avoided, should be avoided, especially if they are quick-wins.
As far as I'm aware Additionally, "other projects have barrel files too" is not really a good argument; it just shows that barrel files are a larger problem in the ecosystem.
Thats fair enough, but I hope you can reconsider :) It'd be unfortunate to have to fork MSW again, I'd much more prefer to use the main package and contribute to improve things here, but the amount of modules we're currently loading in the browser is way too much, and it could be a lot less. Edit: nuance is sometimes hard to express in text, but just want to clarify that this discussion is all in good faith :) |
Beta Was this translation helpful? Give feedback.
-
I thought you were proposing the rework of the public API—the way people import
I fail to see the point here. You can have an integration test of a code that relies on
I don't mean to use other projects' behavior as an excuse to do or not to do something. I was simply bringing it up to illustrate that we should see and learn how others handle things. Some libraries may be times larger than MSW (pre-build) and if there are no issues reported about them slowing things down, then perhaps we are talking about a premature optimization here.
We aren't at the stage of saying yes or no. We are at the stage of discussion and search for a progressive solution to this. Swapping the root-level exports is not a progressive solution. Neither is having both approaches allowed, which will only confuse the developer and their TypeScript: import { http } from 'msw'
import { http } from 'msw/http' You have to also consider how MSW is built to allow proper type references across different entrypoints. It's achieved by those entrypoints referencing the core build. I have a concern that with the This is not an easy problem to solve. Perhaps that's partially why the ecosystem suffers from it so greatly. While addressing this on the package level is likely the "perfect" solution, I encourage to put our effort into exploring the "progressive" solution, which we can adopt since the next release and already bring value.
Circling back to this, do you have any metrics that proves MSW slows you down by loading a bunch of imports from a barrel file? I'd much like to see numbers, and load time numbers, not MBs downloaded (I mentioned this before, downloading 1MB on your local machine may be extremely fast). Without concrete proof of performance degradation, any conversation about performance improvements becomes a premature conversation.
I concur, respect, and appreciate that! The same from my side. |
Beta Was this translation helpful? Give feedback.
-
Updating imports is something that a codemod should be able to do very reliably fwiw. I think there will be people who dislike this and that there’s a risk for a backlash, but I’d personally agree with this change.
|
Beta Was this translation helpful? Give feedback.
-
Has anyone considered importing things from import { http } from 'msw/lib/core/http'
import { HttpResponse } from 'msw/lib/core/HttpResponse' You can follow this pattern for any public export we have. This looks and works quite similar to what is being proposed with I agree with @mattcosta7 that this way the emitted folder structure becomes a part of the public API. However, I'm not going to recommend these imports to anyone by default. In fact, I'd treat relying on them the same as relying on internals. We cannot guarantee their structure so they may break at any point. I understand this isn't nice. But if we are looking at a small percentage of issue occurrence (always happy to be proven wrong, share your use cases!), then the price justifies the gain. |
Beta Was this translation helpful? Give feedback.
-
You can't do this, because of the package exports |
Beta Was this translation helpful? Give feedback.
-
Yeah, we would need to add subpath exports like "./lib/": "./lib/..." |
Beta Was this translation helpful? Give feedback.
-
I have a feeling this is slowly becoming unproductive. I suggest we approach the issue objectively, then define all possible solutions there are, weight their pros and cons, and, eventually, arrive at the right one. I will try my best to do just that. The problem
Proposed solutionsAdd additional entrypointsimport { http } from 'msw/http'
import { HttpResponse } from 'msw/HttpResponse' Pros:
Cons:
Compromise:
{
"exports": {
"http": "./lib/core/http.mjs"
}
}
This also has its disadvantages. Not all tooling understands Another disadvantage is the mental overhead of deciding and maintaining those root-level exports. Should all root-level APIs from |
Beta Was this translation helpful? Give feedback.
-
I want to make it absolutely clear that I have no intention of making rocket science out of this. I'm asking simple questions: how do we detect problematic imports? How do we prevent them as the library develops? Give me an eslint rule and a GitHub action and you have me on board. That which is not automated will never be followed. This has proven true in practice countless of times. I hear a solution but I don't hear how we write a "test" so the issue doesn't happen again. Naturally, such solutions are met with a bit of resistance as they are incomplete by nature. I've also voiced this before, we are discussing both the "perfect" solution as well as the "progressive" solution (if it leads us to the perfect solution that's great). This separation and transition is important because I often miss the context in which something is proposed. Please try to explicitly mention that. Not to mention that often the "right" solution and the "perfect" solution can be different things entirely. You can only discover this after a due discussion. |
Beta Was this translation helpful? Give feedback.
-
I would also add to the pros: "Increased performance"
How is: import { http } from 'msw';
import { setupWorker } from 'msw/browser'; different than: import { http } from 'msw/http';
import { setupWorker } from 'msw/browser'; ? I understand that if your project also happens to use
Im not sure what you mean with this, this is not how export conditions work. Here's an example of export conditions: "exports": {
+ "import": "./index-module.js",
+ "require": "./index-require.cjs"
}, The
I feel like we can safely disregard this, because the library has been using package exports for a long time already now |
Beta Was this translation helpful? Give feedback.
-
To add to the previous post:
This was indeed what I'm suggesting. The only entrypoints to your package are what you define in your package exports. Given that (currently) MSW defines the following keys in the package exports: means that you can only import those things, the only valid module specifiers are: 'msw';
'msw/node';
'msw/native';
'msw/browser';
'msw/mockServiceWorker.js';
'msw/package.json'; Everything else, for example: import { http } from 'msw/http';
// or
import { http } from 'msw/lib/core/http.js';
// or whatever else we decide is the best way to make these publicly available As an aside and just to clarify, I've previously written about how package exports work here and about export conditions here |
Beta Was this translation helpful? Give feedback.
-
First,
I simplified the code for brevity. This is the actual export condition: {
"exports": {
"./http": {
"types": "./lib/core/http.d.ts",
"default": "./lib/core/http.mjs",
"require": "./lib/core/http.js"
}
}
} Is this the suggested solution? Add these for exports like
I wish this was true but we still ship with CJS support and even with modern tooling I've seen cases where simple export conditions were not enough. Tooling still needed root-level placeholder folders like |
Beta Was this translation helpful? Give feedback.
-
See, I misunderstood you entirely. Apologies on my part. I'm happy we are on the same page now, and the discussion helped us arrive there. I think we can extend the |
Beta Was this translation helpful? Give feedback.
-
A few more questions:
|
Beta Was this translation helpful? Give feedback.
-
Always 😜 (jk jk, or well, only partly joking, but I'll forfeit this point)
Pretty much, yes, but there are several ways we can go about this, and we can discuss what would be the best way to go.
As a starting point of discussion to see how we could split things up (note that this is just a suggestion and we can do lots of bikeshedding on how to actually group things): Looking at the barrel file, we export the following: export {
GraphQLHandler,
HttpHandler,
HttpMethods,
RequestHandler,
SetupApi,
bypass,
cleanUrl,
getResponse,
graphql,
http,
matchRequestUrl,
passthrough
}; You could consider creating a export { GraphQLHandler, HttpHandler, RequestHandler, http, graphql }; but this is problematic, because graphql pulls in a lot of modules, this is why I'd consider splitting those up into a export { HttpHandler, http };
export { GraphQLHandler, graphql };
export {
bypass,
passthrough
}; Then we have: export {
cleanUrl,
getResponse,
matchRequestUrl
}; which seem like export {
HttpMethods,
RequestHandler,
SetupApi
}; I'm not sure how relevant those last ones are together, maybe those should be split up differently (or maybe other things should be split up differently), but that would lead to the following added exports: {
"exports": {
"./http": "./http.js",
"./graphql": "./graphql.js",
"./builtins": "./builtins.js",
"./utils": "./utils.js",
"./TODO": "./TODO.js"
}
}
Alternatively, we could just expose a subpath, like @mattcosta7 suggested above, or use a wildcard, like Alternatively, maybe we dont have to create entrypoints for all of those things, maybe entrypoints for |
Beta Was this translation helpful? Give feedback.
-
The difference here is that
To clarify, this is not a browser only-issue, this is how things work in Node as well. If a module imports another module, which imports 5 other modules, which import 10 other modules, you're loading all those modules. This is not a web problem exclusively. |
Beta Was this translation helpful? Give feedback.
-
I've created a PR here: #2004
I've created two packages for this, in case anybody else is reading along/is interested:
|
Beta Was this translation helpful? Give feedback.
-
Lowkey following this discussion because it's super interesting, and because I can tell from experience that barrel files and huge import graphs have a massively detrimental effect on the performance of tools such as Jest where tree shaking and DCE is non-existent. I don't immediately have anything meaningful to add to the barrel discussion, but I am in the middle of migrating a huge test suite to |
Beta Was this translation helpful? Give feedback.
-
I quickly tested this out by linking Taking a test file containing 30 unit tests, using Using hyperfine --warmup 3 'pnpm run test <snip>.spec.tsx'
Benchmark 1: pnpm run test <snip>.spec.tsx
Time (mean ± σ): 9.123 s ± 0.101 s [User: 9.508 s, System: 1.002 s]
Range (min … max): 9.008 s … 9.261 s 10 runs Using hyperfine --warmup 3 'pnpm run test <snip>.spec.tsx'
Benchmark 1: pnpm run test <snip>.spec.tsx
Time (mean ± σ): 8.703 s ± 0.065 s [User: 9.242 s, System: 0.945 s]
Range (min … max): 8.645 s … 8.840 s 10 runs Which in itself is already a very significant improvement. I suspect that this is also a cost you'd pay for every test file rather than every worker, but I'd have to investigate further. edit: Turns out I'm wrong and you pay the cost only once per worker. This is 191 unit tests across 17 files running in band (1 worker): Using hyperfine --warmup 3 'pnpm run test <snip>'
Benchmark 1: pnpm run test <snip>
Time (mean ± σ): 26.707 s ± 0.244 s [User: 29.486 s, System: 2.705 s]
Range (min … max): 26.350 s … 27.173 s 10 runs Using hyperfine --warmup 3 'pnpm run test <snip>'
Benchmark 1: pnpm run test <snip>
Time (mean ± σ): 26.314 s ± 0.359 s [User: 29.278 s, System: 2.569 s]
Range (min … max): 25.937 s … 27.134 s 10 runs One package leading to a +400ms initialization cost is rather 🤯, though. |
Beta Was this translation helpful? Give feedback.
-
Scope
Improves an existing behavior
Compatibility
Feature description
I've mentioned this on discord, but figured I'd create some issues to have this public as well and to better keep track of it. Currently MSW uses barrel files, Marvin Hagemeister wrote a good blog on why they are problematic for dev dependencies: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/
It would be good to create a better separation of entrypoints rather than exposing a single barrel file. We should also look into this for any dependent packages under the
@mswjs/*
namespace.Removing barrel files and a better separation of entrypoints (and avoiding internal modules importing from barrel files) can lead to some pretty significant performance wins
Beta Was this translation helpful? Give feedback.
All reactions