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

Update to work with flask 2.2+ #155

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Conversation

christopherpickering
Copy link
Contributor

Removed _request_ctx_stack.top in favor of reqquest_ctx as shown in https://github.com/pallets/flask/pull/4682/files.

Removed _request_ctx_stack.top in favor of reqquest_ctx as shown in https://github.com/pallets/flask/pull/4682/files.
@nkgilley
Copy link

Bump. @miracle2k any chance you could merge this and create a new release? Thanks!

@paulvoe
Copy link

paulvoe commented Oct 26, 2022

+1

@christopherpickering
Copy link
Contributor Author

btw, this also seems to fix the problem of RuntimeError: Working outside of request context. with the cli build command.

you can install my branch

poetry remove flask-assets # remove if first cause I didn't change the version #.
poetry add git+https://github.com/christopherpickering/flask-assets.git 

@cajual
Copy link

cajual commented Jan 5, 2023

@christopherpickering I'm running into issues with non-static assets. Using npm I import jquery and then add a bundle asset from "node_modules/jquery/dist/jquery.js". This results in a PosixPath length check which fails. This issue only occurs with the new request_ctx class.

@christopherpickering
Copy link
Contributor Author

hey @cajual what is the error message? Was that working previously? There must be another change in flask causing the fail... if you put the full message and how I can reproduce it, I can try to help.

@cajual
Copy link

cajual commented Jan 5, 2023

@christopherpickering sure.

To caveat, I use Quart with flask_patch, but the underlying mechanisms are built on Flask. I bundle my assets in an assets.py and import then to the Flask app, then run assets.init_app(app) in my app factory. All Node modules are imported via npm.

js = Bundle(
    "node_modules/jquery/dist/jquery.js",
    "node_modules/jquery-pjax/jquery.pjax.js",
    "node_modules/bootstrap/dist/js/bootstrap.min.js",
    "node_modules/bootbox/dist/bootbox.min.js",
    "js/app.js",
    filters=(Concat, "jsmin"),
    output="gen/packed.js",
)

Note: Concat is just a simple filter to handle jquery lagging semi-colons.

After making the changes reflected in this to my local installation of flask_assets.py, I started receiving the following output:

File "/Users/.../.venv/lib/python3.11/site-packages/flask_assets.py", line 280, in convert_item_to_flask_url
    filename = filepath[len(directory) + 1 :]
                        ^^^^^^^^^^^^^^
TypeError: object of type 'PosixPath' has no len()

Which is directly related to this segment of code:

    directory, rel_path, endpoint = self.split_prefix(ctx, item)

    if filepath is not None:
>>      filename = filepath[len(directory) + 1 :]
    else:
        filename = rel_path

The flask debugger provides the following information about these vars:

self
ctx
item          node_modules/jquery/dist/jquery.js
filepath      /Users/.../app/static/node_modules/jquery/dist/jquery.js
url_for
directory     /Users/.../app/static
rel_path      node_modules/jquery/dist/jquery.js
endpoint      static

So I attempted to force a len() check on the PosixPath object by casting it to str():

    directory, rel_path, endpoint = self.split_prefix(ctx, item)
    directory = str(directory)

    if filepath is not None:
        filename = filepath[len(directory) + 1 :]
    else:
        filename = rel_path

But now I run into an issue with Jinja2:

File "/Users/.../app/templates/layout.html", line 7, in top-level template code
    {% assets "js_all" -%}
File "/Users/.../.venv/lib/python3.11/site-packages/webassets/ext/jinja2.py", line 194, in _render_assets
    result += caller(entry['uri'], entry.get('sri', None), bundle.extra)
TypeError: can only concatenate str (not "coroutine") to str

So at this point I've just decided there must be a better way to approach this, but it all starts at directory, rel_path, endpoint = self.split_prefix(ctx, item), and it only seems to affect my node_modules paths.

@christopherpickering
Copy link
Contributor Author

@cajual I'm able to use npm packages. Since flask assets servers from the /static folder, I just prefixed jQuery with ../:

Here's the app:

npm init -y
npm install jquery`
poetry init
poetry add git+https://github.com/christopherpickering/flask-assets.git 
poetry add flask
poetry add jsmin
# main.py
from flask import Flask
from flask import render_template
from flask_assets import Environment, Bundle

app = Flask(__name__)

assets = Environment(app)

js = Bundle(
    "../node_modules/jquery/dist/jquery.js",
    filters=("jsmin"),
    output="gen/packed.js",
)

assets.register('js_all', js)

@app.route("/")
def hello_world():
    return render_template('index.html')

and the template

<!-- /templates/index.html -->
{% assets "js_all" -%}
    <script type="text/javascript" src="{{ ASSET_URL }}"></script>
{% endassets %}

then running

poetry run flask --app main run

I wasn't sure what your Concat filter was. I'm guessing the root of the error it is coming from there now. Is it a custom function you have?

@cajual
Copy link

cajual commented Jan 5, 2023

@christopherpickering confirmed, this is specifically a Quart issue. I probably should have double checked the change with Flask instead of Quart. The only difference in the call is async/await, so it must have something to do in the Quart package during this stage. Thanks for the follow-up, I will continue debugging.

@wuttem
Copy link

wuttem commented May 1, 2023

Small improvements that will make this a fully working solution (with CLI):

if not request_ctx is not the correct check if we have a request context (it will raise in the case there is no request context). The check should be done with if not has_request_context() (doc: https://flask.palletsprojects.com/en/latest/api/#flask.has_request_context)

Also it would be better to rewrite the import check

        try:
            from flask import _app_ctx_stack
            app_ctx = _app_ctx_stack.top
            if app_ctx is not None:
                return app_ctx.app
        except ImportError:
            pass

to use has_app_context() (doc: https://flask.palletsprojects.com/en/latest/api/#flask.has_app_context) and app_ctx from flask.globals (doc: https://flask.palletsprojects.com/en/latest/api/#flask.flask.globals.app_ctx). It could be much cleaner with just something like:

if has_app_context():
     return app_ctx.app

@prohde
Copy link

prohde commented Oct 5, 2023

Any news on this one? flask-assets is breaking with Flask 3.0 due to this issue.

@jsalamander
Copy link

@prohde https://pypi.org/project/Flask-Assets2 this fork from @nkgilley seems to be working with Flask 3.0

see #158

@greyli
Copy link
Collaborator

greyli commented Oct 17, 2023

I'm will help to maintain this project. I will fix the tests and set up the GitHub actions first, then handle the pull requests. There will be a new release this week.

@greyli greyli mentioned this pull request Oct 17, 2023
@greyli greyli merged commit 09abde7 into miracle2k:master Oct 17, 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.

None yet

8 participants