worldwise001
commented
5 years ago cc @jejacks0n
Open worldwise001 opened 5 years ago
worldwise001
commented
5 years ago cc @jejacks0n
jejacks0n
commented
5 years ago This is awesome, thanks @worldwise001! I'm in the process of getting this project up to speed and more current, and I'd like to work with you on getting this in there.
My plan is specifically to focus around CSP settings, webpack(er), and reducing the asset pipeline dependency in that process.
Maybe you can help -- I'm seeing calls to require, how is this being provided? is it already in the project?
worldwise001
commented
5 years ago Yes, it should all be already in the project. I just moved some files around so that they are served via the controller instead of being inline JS.
jejacks0n
commented
5 years ago Seems to be! answered my own question. haha.
Ok, so one thing to note here is that right now not all specs / features are being run on CI since I'm still working on getting things polished up. When I get CI back to a place I'm comfortable with can I hit you up to merge master and we can check in at that point?
worldwise001
commented
5 years ago (fwiw I tested this using my own project so I can verify that this causes the CSP errors to go away... I haven't really tested require.js since I don't use that in my project).
worldwise001
commented
5 years ago Sure! Let me know when you're likely going to be able to get CI in a nice place and I can rebase and retest.
jejacks0n
commented
5 years ago Thanks so much @worldwise001 -- I appreciate your time and will reach out in the next couple days as time permits. =)
pamelahowell
commented
5 years ago Assume sauce! Kudos to you both, and sounds like the start of a lovely collaboration.
jejacks0n
commented
5 years ago @worldwise001 -- The test suite is running fully and coverage reports are back in there now. There's a flakey test about code coverage, and I haven't resolved that because it's pretty complex -- and seems like a caching/random spec order issue that manifests somewhere inside sprockets.
You're welcome to rebase master now, and just keep an eye out for that one test. You can just re-run the failing build(s) until it passes -- sorry about that!
Thanks!
worldwise001
commented
5 years ago Fantastic! I'll do that today! (sorry been swamped with other unrelated work).
worldwise001
commented
5 years ago Hrm it looks like something is rewriting all the routes in the test to add /relative in front of it. I imagine this is to test something, but seems to break all the teaspoon routes. Is this intended? I think this is only supposed to kick in for assets?
worldwise001
commented
5 years ago (Any hints on where to look? I tried adding binding.pry to the erbs and those seem to be generating properly, but I couldn't figure out where in the stack frame the rewrite was occurring)
worldwise001
commented
5 years ago never mind, fixed it. javascript_include_tag assumed that it was an asset and so added the relative path accordingly.
jejacks0n
commented
5 years ago Ignore hound.. sorry.
jejacks0n
commented
5 years ago So, do those failures make any sense to you locally?
jejacks0n
commented
5 years ago I changed up the rails versions a bit.
worldwise001
commented
5 years ago The failures are a little baffling to me, and I'm trying to repro it locally. This might take the weekend for me to figure out.
mathieujobin
commented
3 years ago several tests is failing with this changed https://github.com/jejacks0n/teaspoon/actions/runs/657166943 rebased into e85199b
As it is currently set up, if you're running Teaspoon in apps that have strong CSP settings, then all the inline JS pieces will not load causing people to be confused as to why individual tests are not running.
This patch fixes that by moving the files into their own JS files. The trick though was that we still need the suite information for the JS, so what we do here is that we add another route into the controller to custom render the JS based on the suite name (which we also use for the show view anyway).