Skip to content
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

feat: handle routing for starfyre #67

Merged
merged 13 commits into from
Aug 25, 2023
Merged

Conversation

DeleMike
Copy link
Contributor

@DeleMike DeleMike commented Jul 24, 2023

from the test_application/pages where we define some routes we want to have, we generate the corresponding routes in the generated dist/pages folder.

So if we have:

test_application/
    - pages/
        - home.fyre
        - about.fyre

The corresponding generated route will be:

  test_application/
    - dist/
        - home.html
        - about.html

closes #28

from the app_directory/pages, we generate in the generated dist folder, another folder called 'pages' where we all our generated routes will stored
@DeleMike
Copy link
Contributor Author

DeleMike commented Jul 24, 2023

Hey @sansyrox , I hope you are well

So far, I have been able to generate routes defined in the pages directory of the test_application into dist/pages. See here for more info

Am I on the right track?

Also,

Few questions:

  • do the generated routes need to be generated in the test_application/dist/pages directory?
  • if we are generating routes like this, does that mean the contents in the test_application/pages/about.fyre needs to be copied into the file?

PS: I will remove the unwanted print statements as we progress

@DeleMike DeleMike changed the title [WIP] feat: handle routing for starfyre projects [WIP] feat: handle routing for starfyre Jul 24, 2023
starfyre/router.py Outdated Show resolved Hide resolved
starfyre/router.py Outdated Show resolved Hide resolved
starfyre/router.py Outdated Show resolved Hide resolved
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @DeleMike 👋

Great work! 😄 🔥

I have left a few comments. Do let me know if you require any more information from my end 😄

@sansyrox
Copy link
Member

So far, I have been able to generate routes defined in the pages directory of the test_application into dist/pages. See #67 (comment) more info Am I on the right track?

Yess

Few questions:

do the generated routes need to be generated in the test_application/dist/pages directory?

Let us create them in test_application/dist directory beside the index.html file.

if we are generating routes like this, does that mean the contents in the test_application/pages/about.fyre needs to be copied into the file?

Not the contents, but the rendered output. i.e. the components should be resolved like they are being resolved in the __init__.py file.

@DeleMike
Copy link
Contributor Author

I will adjust the folder structure then. Thanks

I need more clarification on the last bit :

Not the contents, but the rendered output. i.e. the components should be resolved like they are being resolved in the init.py file.

What do you mean, please? When we are generating the new routes, the file contents needs to be parsed before it is written to this route?

@sansyrox
Copy link
Member

What do you mean, please? When we are generating the new routes, the file contents needs to be parsed before it is written to this route?

e.g. init.fyre -> init.py -> index.html

Similary, the about.fyre -> about.py -> about.html

Did this make sense? 😅

What I am trying to say is don't copy just the contents but actually resolve the imports like we do in the init file

@DeleMike
Copy link
Contributor Author

Oh definitely makes sense now, thanks for clarifying! I will look into it.

@DeleMike
Copy link
Contributor Author

DeleMike commented Jul 26, 2023

Hello @sansyrox , thanks for your feedback. I have made some progress.

I added the ability for Starfyre to generate the python files for every fyre file in the pages directory. The way Starfyre handles it is, during compilation, we check if there are some fyre files in the pages directory. If there is, we ensure Starfyre tracks it for parsing.

so my next issue is how I go about generating the HTML file.

As a start, I see that generation of the index.html file happens when the main function of Starfyre is called.

My issue is I need the output of return render_root(component) from these generated python files to be sent into the generated html file and not the file contents of that python file. Since I am not triggering any main function for the routes, how do I tackle this issue of the html file generation?

As you said test-application/pages/home.fyre compiled to test-application/build/home.py and the router generates test-application/dist/home.html

The home.py returns render_root(component) which will be the contents to be written into home.html. how do I get the result of the function?

is there another angle I should be looking at?

@sansyrox
Copy link
Member

Hell @DeleMike 👋

So, if you have a look at the __main__.py. You will see that we are importing app function from the __init__.py in the build folder.

Now, this file's contents are generated through the python_transpiled_string function. We have to change the way that function is called.

I hope this helps. We can have a call tomorrow otherwise 😄

@DeleMike
Copy link
Contributor Author

Okay, sounds great! What function are you referring to here?

@sansyrox
Copy link
Member

Okay, sounds great! What function are you referring to here?

@DeleMike i am talking about the 'python_transpiled_string' function

@DeleMike
Copy link
Contributor Author

Thanks, @sansyrox for clarifying. what do you suggest now? Do we want to stop calling that function from the compile function?

starfyre/file_router.py Outdated Show resolved Hide resolved
starfyre/compiler.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/exceptions.py Outdated Show resolved Hide resolved
starfyre/file_router.py Outdated Show resolved Hide resolved
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @DeleMike 👋

Thank you for the PR! I have some suggestions.

Do let me know if you require any more information from my end 😄

starfyre/file_router.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @DeleMike 👋

I have left a few comments. Let me know if you require any more information from me 😄

@DeleMike
Copy link
Contributor Author

We should be keeping the imports like we are currently doing in the __init__.py file. This error is occurring as we don't have the imports.

Thanks for the feedback. Please, I need more clarification. What file are you referring to that I keep the imports like the __init__.py file?

Can you explain more about how the absence of imports affects this issue?

@sansyrox
Copy link
Member

Thanks for the feedback. Please, I need more clarification. What file are you referring to that I keep the imports like the init.py file?

Yes.

Unknown tag: "store". Please review line 1 in your "app" component in the pyml code.

The file is unable to resolve the store variable according to this error.

@DeleMike
Copy link
Contributor Author

Hello @sansyrox, thanks for the clarification earlier.

I have another update. I have been able to tackle the issue where the parser could not resolve the custom components. e.g <store>, <parent>. The issue was not totally the parser's fault.

from .parent import parent
from .store import store

I used the Python debugger to peek deeply into the codebase and I saw that our parser was able to resolve the imports.

The issue was with how we compiled the code. The parser would read the contents of each imported file but it could not resolve if it was a Component. It always classified all our imported components as non-Components and that was the issue.

To resolve this issue and ensure that extract_components was actually able to resolve our custom-defined components children of the Component class.

I resorted to this in the python_transpiled_string function:

def python_transpiled_string(
    pyml_lines, css_lines, js_lines, client_side_python, file_name
):
    file_name = file_name.replace(".py", "").split("/")[-1]
    pyml_lines = "".join(pyml_lines)
    css_lines = "".join(css_lines)
    js_lines = "".join(js_lines)
    client_side_python = "".join(client_side_python)

    root_name = None

    if "__init__" in file_name:
        root_name = "app"
    else:
        root_name = file_name

    if root_name == "app":
        return f'''
from starfyre import create_component, render_root

def fx_{root_name}():
    # not nesting the code to preserve the frames
    component = create_component("""
{pyml_lines}
""", css="""
{css_lines}
""", js="""
{js_lines}
""", client_side_python="""
{client_side_python}
""",
component_name="""{root_name}"""
)
    return render_root(component)

{root_name}=fx_{root_name}()
'''
    else:
        return f'''
from starfyre import create_component, render_root

def fx_{root_name}():
    component = create_component("""
{pyml_lines}
""", css="""
{css_lines}
""", js="""
{js_lines}
""", client_side_python="""
{client_side_python}
""",
component_name="""{root_name}"""
)
    return component

{root_name}=fx_{root_name}()
rendered_{root_name} = render_root({root_name}) # see the change here
'''

So now, we ensure every custom-defined is classified as a Component.
And to add to it, instead of handling the rendering in the generate_pages function, we now get the output from other components with rendered_{root_name} = render_root({root_name}). The result of rendered_{root_name} will have the HTML output for the component.

With this, we make each page a root component and ensure that the parser can resolve the components properly.

starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
@DeleMike
Copy link
Contributor Author

DeleMike commented Aug 17, 2023

Hey @sansyrox, thanks for your feedback! In this latest update, I have answered the questions you asked and I have reduced the starfyre/__main__.py file contents by introducing a template file starfyre/main_template.py. This will be used to enhance code readability.

starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
starfyre/__main__.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @DeleMike 👋

The __init__.fyre should be in the pages folder. That would also change some logic in the import resolution 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give this file a more specific name please? main_template is a bit too generic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeleMike this too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,76 @@
def get_main_template(user_routes, path):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeleMike 👋

These comments need to be addressed too 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

starfyre/__main__.py Outdated Show resolved Hide resolved
@sansyrox
Copy link
Member

Hey @DeleMike 👋

Thank you for your updates. I have some suggestions. Please do let me know if you have any questions 😄

@DeleMike
Copy link
Contributor Author

Hello @sansyrox , thanks for the feedback, I have applied them. The most notable will be ensuring that __init__fyre exists in the pages directory amongst other changes. Thanks!

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , @DeleMike 🔥

Great work. I just want to do a final local test and will merge it afterwards

@DeleMike
Copy link
Contributor Author

Thanks for the feedback @sansyrox. Fingers crossed! 🤞🏾

@sansyrox
Copy link
Member

All LGTM. Great work @DeleMike 🔥

@sansyrox sansyrox merged commit a405bc0 into sparckles:main Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide on the folder structure
2 participants