-
-
Notifications
You must be signed in to change notification settings - Fork 474
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Avoid barrel file exports #1984
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. |
Here's the PR that removes imports to a barrel file by an internally used module #1987 |
Fixed by #1987. 👏 |
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 |
@thepassle, do you have the internal build entrypoints in mind (i.e. |
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 :) |
@thepassle what tool did you use to visualize the imports ? |
Madge |
@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. |
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 |
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, |
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). |
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 :) |
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. |
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.
|
I wonder where does this change end though? We have about 20-40 things exported from the root of I've not seen a single package do that in all the time I'm doing open source. Does anybody have an example of a package solving this problem that way, perhaps? |
You have to consider the implications of this change that live outside of your code base too. Educational materials, articles, examples, docs. It's a gigantic change. It has to have gigantic benefits. As of now, I'm yet to see evidence that would suggest that does.
This is quite subjective and rather insignificant gain from the library's perspective.
They are. But MSW isn't really meant to be bundled as you don't include it in your production build. I may presume we are talking about bundling the app for testing, and in that case I still need to see any tangible performance metrics that would suggest MSW causes any test run time degradation. |
Conceptually, I like the idea of removing barrels, and very much agree we shouldn't import through them in the library itself. But it's hard to have a conversation in the abstract, when it's not clear what level of size could be avoided if we did remove them Tools like material-ui and lodash tend to provide dual import apis, with both subpath exports and a barrel file. Something like that might make sense here, but the effort/value tradeoff seems high for now? I think spliting out the handlers like |
Yes I respect that, just my 2 cents for a personal preference of not using barrel files in general, while recognizing that the trade offs in this case involve way more data points. |
This is a super naive setup for testing, was just curious since I've been playing with node's native test runner and have experienced issues with barrel style imports adding a non-trivial amount of overhead to each test and overall test run. Death by 1000 cuts. In this case it might be negligible, but not representative of real world test setups where maybe there is more of a performance hit. Ran on a ~2 year old apple m1 max MacBook pro. https://github.com/derekr/mswtest
|
it would be a fairly large breaking change to remove the index file as a whole, so i think it makes sense to keep that around for sake of not having to update all the docs, references, etc. consumers who bundle or are entirely server-side can continue using the "pretty" import too (the bare specifier). rxjs and many others provide groupings of exports in individual entrypoints. it may be worth doing something similar here if the current index affects unbundled consumers too much (though not a small job since discussion needs to happen to decide what those groupings are) alternatively, you could probably just update the export map to export all the individual files so consumers have the option to directly import individual modules if they really want to. either way, seems no loss to existing consumers who don't care about this stuff (perf, size, primary entrypoints, etc will all be the same). |
To clarify, you don't have to manually add entrypoints for every single file, you can just use a wildcard, here's how I do this in |
@thepassle, thank you for providing some concrete metrics.
Based on the screenshot, the page still loaded under 400ms. I'd consider this fine in terms of developer experience. I also wonder what the browser should do with third-party modules it fetches over HTTP. I'd expect it to cache them for they are never meant to change (you don't own them so for you they are de-facto static assets that can be cached by package version). With caching, this problem really happens just once per dependency update. But I suspect the whole direction is so new there's no actual standardization as to how the module fetching should behave (happy to be proven wrong, please post links).
What will be the deciding factor for those entrpoints? Do we base those primarily on the amount of code each entrypoint introduces? Then I wonder how you intent to keep that amount of code in check so a new feature would not blow up a particular entrypoint. Here we enter an even more uncharted land. Changes like this cannot really happen in a sustainable fashion unless there's tooling around them. Tooling to automat, detect, report enrtypoint size changes, tooling to work with the code in the context of where that code ends up, etc. We haven't even touched on this in the discussion but the maintainer's cost for this change is stupendous. This brings back memories to proper package bundling and how I have to design a bunch of scripts just to make sure my package isn't broken when published and provides the right
Even the introduction of the Fetch API has been met with a palpable struggle from the users. And I consider that a mild API change, to be frank. If the choice we make will negatively impact the end developer, then it's a choice that has to be considered with utmost thought. I hear codemods being mentioned and I want to clarify on them. Codemods are great. But you need to know how to use codemods. Then, codemods can produce broken code. Now, you need to be aware how it's broken and have the capacity to fix it by yourself. This discussion has no doubt gathered a lot of engineers who tackle quite complex tasks. But you have to acknowledge that not every engineer is used to this level of complexity. For many, even a trivial change can be a significant time investment, and so it has to have solid justification. ProposalSo, there are a couple of major factors that are present in the "problem":
May I propose an alternative approach here? It won't solve the problem but it will mitigate the symptoms and bring immediate value without being a breaking change. Why don't we minify the browser bundle? We still ship the source code and sourcemaps so debugging won't suffer. With minification, we can get a significant reduction of the download JavaScript in terms of file size.
|
This is extremely unlikely if the only thing that changes are imports. Also, this would only even be relevant if we would do a breaking change, which is not what we're suggesting; we're suggesting adding additional entrypoints. We can keep the current barrel file in place, for the time being.
How is adding additional, optional entrypoints that would lead to better performance a struggle for users?
There are several ways we can go about this (which we can all discuss in detail, and some options have already been mentioned in this issue), that, frankly, I really don't feel put a stupendous maintenance burden on the project, I'm really not sure where you're getting this from.
Because the browser bundle is not the problem, I feel I've mentioned this several times. The All in all, I really fail to understand the reluctance to this request, and I haven't really heard any solid arguments to not consider adding some additional, optional entrypoints for people who prefer to have performant local development/unit tests. I'd really love to understand better why you're so hesitant about this. |
i feel like this is too easily overcomplicated. there seem to be two options:
the former takes effort to spec it out, decide what the correct entrypoints are, etc. it also then takes extra effort to maintain and keep that in sync. it is what libraries like rxjs do and works well, but is understandable if the maintainers here don't want to take that on. the latter is a one-time piece of work which introduces no extra burden, no breaking changes, no rfc-level discussions needed. though it does require more work from the consumer to import the exact paths of the modules they want (rather than it coming from a "nice" grouped entrypoint). if there's this much concern about the former, i'd just update the export map and note it down in the docs somewhere. i could be missing something, of course. let me know if i am or if i misunderstood |
This would make any change to internal folder/file structure becomes a breaking change to the public api, so I don't think that's actually as simple as you suggest |
is that not just a matter of definition? you can define that the individual modules may change any time, so are not part of the "public API" (i.e. the exported namespace). consumers can choose to depend on them but at their own risk. i've seen a few packages do this before (and, in fact, pre-export-maps most packages changed files often without a breaking change) |
I might have expressed my point weakly because this isn't an appropriate solution: import { http } from 'msw'
import { http } from 'msw/http' This contributes to a terrible experience for everyone. This confuses TypeScript and your IDE, and I can hardly imagine a proper way to document it not to cause further confusion. This is the direction I'm encouraging us to avoid, not adopt.
I will give you an example. Someone adds a feature. The code for that feature adds a whole new block of modules to the library. The person forgets to care about imports, and suddenly, introduces a barrel file. The person reviewing the change misses this also. Suddenly, we are back at square one. It doesn't matter if it's a top-level barrel file or a barrel file nested 50 modules deep. This is why I stress that without a proper tooling to automate this, this will always be a burden on maintainers. You look at this as a quick solution to a problem. I look at this as a solution I would have to maintain for years. The difference in commitment level is what spawns this discussion. We are discussing a complex problem of the entire ecosystem that nobody has quite solved yet. You must forgive any reluctancy on my part.
With that suggestion, I have proposed to bundle everything for the browser. No more browser/core separate files. One
Not all apparent solutions are the right solutions. I believe you see a problem and see a solution for that problem. I also see numerous implications to that solution, experience-wise, runtime-wise, and documentation-wise. I have employed my expertise on this kind of thing for years, and I'm not going to sway now. I'm a strong opponent of workaround-driven development. The To be absolutely frank, I'm also a strong proponent of "measure before you cut". I want to hear more people having this issue. Because if things have been working fine for them for 5 years, there is no issue in practice. |
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. |
You can't do this, because of the package exports |
Yeah, we would need to add subpath exports like "./lib/": "./lib/..." |
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 |
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. |
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 |
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 |
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 |
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 |
A few more questions:
|
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 |
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. |
I've created a PR here: #2004
I've created two packages for this, in case anybody else is reading along/is interested:
|
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 |
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. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
The text was updated successfully, but these errors were encountered: