-
Notifications
You must be signed in to change notification settings - Fork 522
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
Usage report - concatjs #1026
Comments
Thanks for keeping such great notes! What is the next step here? Seems like we need to extract some of this into docs, and file issues for the rest? |
I'm not sure what to do, @achew22 do you think we should just transcribe your notes to the wiki or something? I don't want to lose them. |
What would your thoughts be on moving it into the concatjs directory as a checked in |
This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs! |
No longer in scope for rules_nodejs |
I just finished getting ConcatJS set up in my project and while the memories are fresh I wanted to document the things that left me slightly puzzled. I think I'm probably the first person to do this, so it might give you some direction on how the integration might be improved.
Thanks. I'm really happy that this exists. Writing my own concatjs would have been a PITA. Looking through the code it seems like there are some tricky edge cases that would have tripped me up. Thanks!
Visibility is a problem. You're in an interesting place where it doesn't make sense for someone using the default go toolchain to import concatjs. What would they do with it? As such the Bazel visibility is the one that really matters. Unfortunately it is currently set to
//internal:__subpackages__
. This is almost a showstopper except for the fact that you can addbuild --nocheck_visibility
to your projectsbazel.rc
. This seems like the most important thing to fix.Target naming. I think the
rules_go
community is starting to settle around using gazelle to manage things. As such the target name of@build_bazel_rules_typescript//internal/concatjs
is not what is expected by Gazelle. It expects@build_bazel_rules_typescript//internal/concatjs:go_default_library
. Further there is a mismatch between the workspace namebuild_bazel_rules_typescript
and the importpath definition ofgithub.com/bazelbuild/rules_typescript/internal/concatjs/concatjs
. For gazelle to import that correctly, it would need to have importpath set toimportpath = "bazel.build/rules_typescript/internal/concatjs/concatjs"
. Also, the duplication ofconcatjs
on the import path at the end appears to be an artifact of the Google3 implementation of rules_go that was not carried forward during the open source rewrite. Interestingly, I think this is a thing the Gazelle team is aware of and would like to address. They have a tracking bug Index build files in external repositories bazel-gazelle#12 to make gazelle work properly with this code, but I still think it would be better to run Gazelle on the code during the export process to align it with the OSS community's expectations better.Content security policies.
concatjs
useseval
to load the scripts into the page. That's a totally fine thing to do, but it did interfere with the CSP I already had on the page. It might be worth noting that CSPs are essentially incompatible with this approach of doing the devserver and that you will have to add a flag to disable CSPs as necessary.Serving paths. When I initially plugged in the
concatjs.ServeConcatenatedJS
handler, I put it at/js/concat.js
. Earlier in the file I had registered arouter.PathPrefix("/js/").Handler(...)
. Turns out you can't graft into the hierarchy over aPathPrefix
. Probably not a concern for most people, but it might be worth noting that in the docs.preScripts
. I copied the style of having a list ofpreScripts
that were passed intoconcatjs
rather than including scripts in the page. I needed to manually include RequireJS, which I did by adding adata
dependency on@build_bazel_rules_typescript_devserver_deps//:node_modules/requirejs/require.js
and with the following block of go, before my call toconcatjs.ServeConcatenatedJS
:postScripts
. Since I used the style ofpreScripts
, it only seems reasonable that I would use the style ofpostScripts
. I needed to addpostScripts = append(postScripts,
require(["my/entry/point"]);)
before the call to the handler as well.The correct base dir. It was a little tricky figuring out the correct working dir inside my server.
tags
. I also needed to add the following tags to the target in order to have iBazel do "the right thing(TM)".Depending on the right data. When it came time to depend on the correct resources, I was left a little befuddled. Initially I thought I would only need to depend on the
ts_library
(but that doesn't generate a .MF file). Then I thought that I would only need to depend on thets_devserver
and that it would provide, transitively, access to the compiled .js files. That was not the case. I needed to depend explicitly on both.Sorry to wall of text, but as I said I wanted to write down all the little pains that you can only see the first time you do an integration.
All and all I would say, GREAT SUCCESS! 🎉. ConcatJS seems to be working great in my project and I'm really enjoying the diminished compile times and iteration speed. Kudos!
The text was updated successfully, but these errors were encountered: