-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Idea for even more streamlining #12
Comments
Good discussion here.
I don't see much benefit to hiding the import statement - I think it's easier to understand and debug when you can see the import statement in code instead of it being inserted through a webpack plugin. Putting it in a webpack plugin could result in runtime errors that reference lines of code that aren't even present in the source code, which would be confusing.
I don't understand what you mean here - could you clarify? Having only a single public path is not a problem for systemjs modules. The problem that systemjs-webpack-interop solves is to allow for webpack code splits to load from urls relative to the entry module instead of relative to the web page (which is what webpack does by default).
Thanks for sharing that PR - I was unaware of it. The purpose of the pull request seems to be to modify the chunk file name, not to modify the public path. If I'm understanding the pull request correctly, this isn't a replacement for public path, but rather just a way of changing what comes after the public path. I don't see any advantage of using it within systemjs-webpack-interop. If I'm missing something here, though, let me know. |
I will try to outline my thoughts. (I don't have strong opinions and I am still learning this technology so this is a bit of an exercise for me to clarify my thoughts :-) ) An ideal setup would:
To accomplish this goal the ideal scenario would be:
Rereading the PR I think you are correct. Sorry, wishful interpretation.
This is what I understood. I was trying to say that I think this is to work around the webpack 'limitations' that webpack publicpath doesn't support functions and that webpack doesn't allow for relative urls (relative to source like imports not relative to page).
This is a bit of a cosmetic personal taste but I will try to explain my reasoning a bit more. Requiring an import for an unreferenced library as the first import seems incorrect to me. (while I understand why it is needed and it is a very elegant implementation it feels like a 'side effect' api.) Adding to a webpack plugin could cause a runtime error but I don't think this is a problem for a) webpack (and babel) do lots of transformations already additions/deletions, etc, and these can be managed via sourcemaps. b). If there is a runtime error it would be around loading a dynamic module and having the error point to the code that sets the publicPath could actually complicate, confuse uses who don't understand the internals (ideally most people) since the error should really show up when doing the actual import (not the setPublicPath). Finally, to be very clear. This is a nit. I am finding the single-spa eco system (build/config scripts, library apis, dev-tool, support, and documentation) to be wordclass! |
I honestly don't have a strong preference between webpack plugin and a code library. I see some advantages to both the current version and a webpack plugin. If you'd like to work on a webpack plugin to do the same thing as systemjs-webpack-interop, I'd support that. If you like the plugin to be in the single-spa github org and maintained there, I'd be happy to create an empty repo and grant you write access. If you'd prefer it be under a different account, that's completely fine, too. If it turns out to be easier for people to use the plugin, perhaps we could modify create-single-spa to use it. |
I have not done a webpack plugin before but I will add this to our internal development backlog as a 'nice to have'. |
👍 i'll keep this open, and perhaps one day I'll try it out, too. Webpack plugins are not as scary as they sound - happy to help guide anybody who wants to look into it. |
I had time today to implement this, so I did so in #14. I think this will be a good option for a lot of people using this. |
@joeldenning, (I just discovered single-spa so working through a POC).
I see with the latest changes you have streamlined the setting of the relativePath to just a single line (Very nice!).
As a thought even this line could be removed (or hidden). If there was a webpack plugin that add this line as the first line to all entrypoint files. (And of course the webpack plugin registration could be wrapped in 'singleSpaDefaults'.
As another 'thought'; this all feels like a workaround for webpack allowing have a single global publicPath. Webpack 5 has a merge request for resolving paths using a function. Would this be a better solution than updating the global variable on load? (Not fully aware of either implementation but want to propose in case you were unaware).
https://github.com/webpack/webpack/pull/8462
Anyway not a specific request but just wanted to contribute a few ideas.
The text was updated successfully, but these errors were encountered: