Closed Dale-Black closed 4 months ago
Didn't want to derail #122 so posted here. Were there changes to @get
calls? My original code is now longer working after updating to master
Unfortunately, it was not possible to extend the Base.get
with the @oxidise
macro as that broke precompilation. There are three solutions which you can do as the user of the new @oxidise
macro:
Base.get
;module OxygenInstance using Oxygen; @oxidise end
import .OxygenInstance: put, post # and etc
Base.get(func::Function, path) = OxygenInstance.get(func, path)
Oxygen
and call @oxidise
explicitly after setting that get
method will be extended:
import Base: get
import Oxygen; Oxygen.@oxidise
I prefer the second approach with explicit imports of @get
, @post
, and @put
, for which the third line can be skipped.
There does not look much to do without a breaking change. Perhaps we could export the functional syntax get
, post
, put
only when @oxidise
macro is used and thus import Base: get
could be included.
Ahh yes I like the second option, that works great. This is pretty exciting! It's still a little janky doing frontend but "full stack" now feels doable! Great, work on the HMR stuff, thank you.
Very cool animation. Perhaps @ndortega can add it to the readme!
Thanks! I just added an updated gif to my HTMLStrings.jl readme here (https://github.com/Dale-Black/HTMLStrings.jl/blob/main/images/todo_app.gif) if it's of use.
If there is any interest, I would be happy to talk more about integrating HTMLStrings with Oxygen.jl even tighter. It's a super simple templating DSL. I know I would love to have Oxygen.jl and HTMLStrings.jl work together to build static HTML pages
Hi @Dale-Black,
Do you mind checking out and testing this branch: feature/oxidise-context-refactor I'm planning on merging it soon and wanted to see if this impacted your workflow. In my testing, this branch no longer requires you to do the following:
import Base: get
import Oxygen; Oxygen.@oxidise
You should be able to import Oxygen like you did before and call the new macro like below
using Oxygen; @oxidise
You can try it out by running the following
pkg> add https://github.com/OxygenFramework/Oxygen.jl#feature/oxidise-context-refactor
Let me know what you think. Fair warning, I'm planning on merging this branch sooner rather than later to get ready for a release sometime next week.
@ndortega the warning appears when @oxidise
macro is used from within the module during precompilation:
Info Given ModuleA was explicitly requested, output will be shown live
WARNING: Method definition get(Function, String) in module Oxygen at /Users/jerdmanis/BtSync/Projects/Oxygen.jl/src/methods.jl:207 overwritten in module ModuleA on the same line (check for duplicate calls to `include`).
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
? ModuleA
[ Info: Precompiling ModuleA [d71a4861-d403-46ba-8c26-d84b084f954f]
WARNING: Method definition get(Function, String) in module Oxygen at /Users/jerdmanis/BtSync/Projects/Oxygen.jl/src/methods.jl:207 overwritten in module ModuleA on the same line (check for duplicate calls to `include`).
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
The change you made with import Base: get
in oxidise macro is equivalent to putting that statement in methods.jl
. This was the reason why it was put at the top of the scope, keeping the get
to work with using Oxygen
as previously and thus only if one uses @oxidise
macro behaviour is different.
Hi @JanisErdmanis,
Thanks for bringing that up, can you show me the steps to reproduce that precompilation warning?
Don't worry I also so your comments on the pr, rest assured I have good reasons for making those changes, but I won't be able to respond till this afternoon.
Create a package OxygenTest
with the following contents:
module OxygenInstance
using Oxygen
import Base: get # This simulates the inclusion of this statement in `@oxidise` macro
@oxidise
@get "/inside" function(req::Request)
return "Hello from Inside now 232332"
end
@get "/test" function(req::Request)
return "Tes"
end
end
Use it as a module:
using OxygenInstance
function ReviseHandler(handle)
req -> begin
Revise.revise()
invokelatest(handle, req)
end
end
server = OxygenInstance.serve(port=2345; middleware=[ReviseHandler])
Hi @Dale-Black,
Do you mind checking out and testing this branch: feature/oxidise-context-refactor I'm planning on merging it soon and wanted to see if this impacted your workflow. In my testing, this branch no longer requires you to do the following:
import Base: get import Oxygen; Oxygen.@oxidise
You should be able to import Oxygen like you did before and call the new macro like below
using Oxygen; @oxidise
You can try it out by running the following
pkg> add https://github.com/OxygenFramework/Oxygen.jl#feature/oxidise-context-refactor
Let me know what you think. Fair warning, I'm planning on merging this branch sooner rather than later to get ready for a release sometime next week.
Hi, sorry for not replying last week. Is this still open? If so, I can give it a quick test
Hi @Dale-Black,
I've reverted the breaking things I introduced, but did push some modifications. These changes went live in v1.5.0 let me know if you face any issues in this latest version.
Amazing, this just works now
module TodoApp
import Oxygen: @get, @post, @delete
Oxygen.@oxidise
import HTTP
using HTMLStrings
include("routes/index.jl")
include("routes/todo.jl")
end # module
Fantastic work and great library
I just had an unexpected epiphany. The experience with respect to the get
method shadowing can be made better. To avoid the issues where shadowed get
method overtakes Base.get
method we could do the following:
get(collection, key) = Base.get(collection, key)
get(func::Function, path::String) = route([GET], path, func)
get(func::Function, path::Function) = route([GET], path, func)
@ndortega can we add the additional method for get?
Great Idea! I'll create some tests with both get() functions to make sure the shadowing works as expected
Note that you may need to add get(collection, key) = Base.get(collection, key)
within the @oxidise
macro so that it would not conflict with import Base: get
which would be overwritten otherwise. Or alternativelly remove import Base: get
which shall also work as long as user does not reference get
method before Oxygen is imported.
Originally posted by @Dale-Black in https://github.com/OxygenFramework/Oxygen.jl/issues/122#issuecomment-1946322439