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: support frontend plugins via env.config.jsx #240

Merged

Conversation

arbrandes
Copy link
Collaborator

@arbrandes arbrandes commented Nov 20, 2024

This provides a mechanism to configure frontend plugins using a new PLUGIN_SLOTS filter and a series of patches that modify a base env.config.jsx, in such a way that multiple plugins can take advantage of the file (including for purposes beyond frontend plugins) without clobbering each other.

(This is the third take. Prior: #233, #234)

@arbrandes arbrandes requested a review from regisb November 20, 2024 19:35
@arbrandes
Copy link
Collaborator Author

@regisb, how about this?

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

I think this is a great improvement, and I only have minor comments. Looking forward to merging this.

};

{%- for app_name, app in iter_mfes() %}
if (process.env.npm_package_name == '@edx/frontend-app-{{ app_name }}') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems brittle. Will this work for 3rd-party hosted packages? Is there no other way to have access to the MFE app name? For instance process.env.APP_ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I initially considered APP_ID but didn't go for it because it's not really used in the upstream MFE repos.

But then again, you're right, if somebody adds an MFE that doesn't follow the frontend-app convention, it wouldn't work here. Plus, APP_ID is set in the Dockerfile for every MFE anyway. I'll see if it works.

README.rst Outdated
/* Hide the default footer. */
op: PLUGIN_OPERATIONS.Hide,
widgetId: 'default_contents',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a nitpick: I suggest having separate items for every plugin slot. I.e:

 PLUGIN_SLOTS.add_items(
    ("all", "footer_slot",
        """
          {
            /* Hide the default footer. */
            op: PLUGIN_OPERATIONS.Hide,
            widgetId: 'default_contents'
          }),
    ("all", "footer_slot", """..."""),
    ...
)

In particular, with this approach we don't have to be too careful with trailing commas.

This is a minor concern and should not be blocking this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As the env.config.jsx template stands now, when targetting "all" MFEs people will always have to add trailing commas, regardless of whether they're in separate filter items or not.

I agree this is less than ideal, though. Plus, this makes me realize the current version of the mfe-specific slot configuration has a bug (because I was still thinking in hard-coded slot names mode). I'll figure something out.

@@ -90,6 +89,14 @@ def get_mfes() -> dict[str, MFE_ATTRS_TYPE]:
return MFE_APPS.apply({})


@functools.lru_cache(maxsize=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to use tutor.hooks.lru_cache (and fix get_mfes above)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, will do!

"""
This function is cached for performance.
"""
return [i for i in PLUGIN_SLOTS.apply([]) if i[0] == mfe_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

PLUGIN_SLOTS.iterate() is syntactic sugar for PLUGIN_SLOTS.apply([]).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice! I'll make use of it.

tutormfe/templates/mfe/build/mfe/env.config.jsx Outdated Show resolved Hide resolved
"""
Yield:

(mfe_name, slot_name, slot_config)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are never using mfe_name in the templates. Does that mean that we should simply not return it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I left it in just in case. But yeah, there's less of an argument for it if the getter function only works when defining an MFE (or "all"). I'll change it.

}
{%- endfor %}

{{- patch("mfe-env-config-tail") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: The "-tail" and "-head" suffixes are not used for other patch statements in Tutor. For instance, the patches in openedx/Dockerfile use "-pre", "-post", "-final". I'm not sure which convention is best, but we should probably follow the existing one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I struggled with the names here, clearly. I have a different take that I'll push shortly.

@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch from 14a46cf to 6d749fd Compare November 21, 2024 18:02
@arbrandes
Copy link
Collaborator Author

@regisb, ready for a second round.

@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch from 6d749fd to 3bc8bed Compare November 21, 2024 18:18
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Stellar!

{{- patch("mfe-env-config-buildtime-imports") }}

function addPlugins(config, slot_name, plugins) {
if (slot_name in config.pluginSlots === false) {
Copy link
Contributor

@regisb regisb Nov 22, 2024

Choose a reason for hiding this comment

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

nitpick: Is this idiomatic? Shouldn't it be if(!(slot_name in config.pluginSlots))? Or if(config.pluginSlots[slot_name] === null)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think only if (slot_name in config.pluginSlots) is fine.

Copy link
Collaborator Author

@arbrandes arbrandes Nov 22, 2024

Choose a reason for hiding this comment

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

On syntax: when testing for whether properties exist or not, Javascript is a minefield. You have to make sure you're not testing the value of the property if it exists. This is one of the reasons why the in operator came about.

And truthiness is also a minefield in Javascript. === false is the only way to be absolutely sure. It's why MDN uses it in their example.

Either way, we do have to use the negative case, here. It avoids duplicating code.


{{- patch("mfe-env-config-buildtime-definitions") }}

async function getConfig () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change this function name? It's conflicting with import { getConfig } from '@edx/frontend-platform';.

Copy link
Collaborator Author

@arbrandes arbrandes Nov 22, 2024

Choose a reason for hiding this comment

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

Sure. But I don't know what the behavior is going to be if you call frontend-platform's getConfig while actually setting config.

In any case, I'll just call it setConfig, then.

{{- patch("mfe-env-config-runtime-definitions") }}

{%- for slot_name, plugin_config in iter_plugin_slots("all") %}
addPlugins(config, '{{ slot_name }}', [{{ plugin_config }}]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason for adding plugin_config object one by one?

Could we pass array for one plugin_slot (keeping in mind multiple plugins can have same slots)?
image

Like this:

PLUGIN_SLOTS.add_items([
        (
            mfe,
            "footer_slot",
            """
            [
    {
        op: PLUGIN_OPERATIONS.Hide,
        widgetId: 'default_contents',
    }
    {
      op: PLUGIN_OPERATIONS.Insert,
      widget: {
        id: 'default_contents',
        type: DIRECT_PLUGIN,
        priority: 1,
        RenderWidget: <Footer />,
      },
    },
    {
      op: PLUGIN_OPERATIONS.Insert,
      widget: {
        id: 'read_theme_cookie',
        type: DIRECT_PLUGIN,
        priority: 2,
        RenderWidget: AddDarkTheme,
      },
    },
  ]"""
        ),
    ])

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I just realized that objects are already passing in array. I need to pass without array brackets:

PLUGIN_SLOTS.add_items([
        (
            mfe,
            "footer_slot",
            """
            
    {
        op: PLUGIN_OPERATIONS.Hide,
        widgetId: 'default_contents',
    },
    {
      op: PLUGIN_OPERATIONS.Insert,
      widget: {
        id: 'default_contents',
        type: DIRECT_PLUGIN,
        priority: 1,
        RenderWidget: <Footer />,
      },
    },
    {
      op: PLUGIN_OPERATIONS.Insert,
      widget: {
        id: 'read_theme_cookie',
        type: DIRECT_PLUGIN,
        priority: 2,
        RenderWidget: AddDarkTheme,
      },
    },
  """
        ),
    ])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are different ways you can load the configuration in. @regisb suggested we document it one-by-one to avoid issues with trailing commas. Though in this latest iteration of env.config.jsx it doesn't really matter if there are trailing commas or not, I guess it keeps it simple.

Otherwise, yes you can totally use a single add_item() with a string that has multiple ops for one slot.

README.rst Show resolved Hide resolved
README.rst Outdated
Comment on lines 564 to 569
.. code-block:: javascript

const mymodule1 = await import('mymodule1');
const { mycomponent1, mycomponent2 } = await import('mymodule2');

Note that if any module is not available at runtime, ``env.config.jsx`` execution will fail silently.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update the documentation to specify that if a component is exported as a default export, it should be accessed using Component.default when dynamically imported? This clarification will help ensure proper usage and avoid potential errors during runtime.

I was testing with indigo and here's my file:

// import Footer from '@edly-io/indigo-frontend-component-footer';

...

if (process.env.APP_ID == 'learner-dashboard') {
const Footer = await  import('@edly-io/indigo-frontend-component-footer');
...
{
      op: PLUGIN_OPERATIONS.Insert,
      widget: {
        id: 'default_contents',
        type: DIRECT_PLUGIN,
        priority: 1,
        RenderWidget: <Footer />,
      },
    },
...
}

It is throwing error Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.

If I use const { Footer } = await import('@edly-io/indigo-frontend-component-footer');, it throws this error Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports..

The footer is exported as default. export default Footer;. The errors are generated when I shift to dynamic import await (...). Could we add in doc that we have to use Footer(Component).default in case of dynamic imports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'll point to the MDN documentation. They mention that you have to use the default key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They actually document a better way further down in the page, which is what I ended up putting up as an example.

@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch from 3bc8bed to 7daaeb5 Compare November 22, 2024 14:42
@arbrandes
Copy link
Collaborator Author

I addressed the latest comments: feel free to merge it if it looks good, or let me know if not. :)

@hinakhadim
Copy link
Contributor

Just a quick heads-up from my side to proceed with merging it.

@regisb
Copy link
Contributor

regisb commented Nov 25, 2024

@DawoudSheraz can you please merge and rebase the sumac branch on top of nightly?

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

I will test this branch locally with indigo nightly. Hina is on leaves this week, she mentioned that this can break indigo. I would verify that before merging.

@DawoudSheraz
Copy link
Contributor

I will test this branch locally with indigo nightly. Hina is on leaves this week, she mentioned that this can break indigo. I would verify that before merging.

I tested this locally and indigo's dark theme toggle is not working as expected. It is not appearing consistently across pages (for instance, it does not appear on the learner dashboard). Even when it appears, the toggle does not retain its value when navigating to different pages. Hina mentioned that this could impact indigo.

I am ok with merging considering Hina will be back from leaves next week and can look into updating Indigo.

@arbrandes
Copy link
Collaborator Author

Let me know if I can help with anything. We should probably try and get this (or something like this) merged this week so it comes out with Sumac.

@regisb
Copy link
Contributor

regisb commented Dec 2, 2024

@hinakhadim friendly nudge :)

@hinakhadim
Copy link
Contributor

hinakhadim commented Dec 2, 2024

Yeah, On it. I was stuck into an issue regarding this. I was facing errors in only ora-grading MFE. All MFEs were working fine with the same env.config.jsx (those who don't have indigo footer installed like authn). But that is now resolved and will create a PR in few hours.

Errors:

ERROR in ./env.config.jsx 53:17-68
Module not found: Error: Can't resolve '@edly-io/indigo-frontend-component-footer' in '/Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading'
resolve '@edly-io/indigo-frontend-component-footer' in '/Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading'
  Parsed request is a module
  using description file: /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/package.json (relative path: .)
    Field 'browser' doesn't contain a valid alias configuration
    resolve as module
      looking for modules in /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src
        single file module
          using description file: /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/package.json (relative path: ./src/@edly-io/indigo-frontend-component-footer)
            no extension
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer doesn't exist
            .js
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer.js doesn't exist
            .jsx
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer.jsx doesn't exist
            .ts
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer.ts doesn't exist
            .tsx
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer.tsx doesn't exist
        /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/src/@edly-io/indigo-frontend-component-footer doesn't exist
      looking for modules in /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules
        single file module
          using description file: /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/package.json (relative path: ./node_modules/@edly-io/indigo-frontend-component-footer)
            no extension
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer doesn't exist
            .js
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer.js doesn't exist
            .jsx
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer.jsx doesn't exist
            .ts
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer.ts doesn't exist
            .tsx
              Field 'browser' doesn't contain a valid alias configuration
              /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer.tsx doesn't exist
        /Users/hina.khadim/Documents/tutor-redwood-indigo/frontend-app-ora-grading/node_modules/@edly-io/indigo-frontend-component-footer doesn't exist
      /Users/hina.khadim/Documents/tutor-redwood-indigo/node_modules doesn't exist or is not a directory
      /Users/hina.khadim/Documents/node_modules doesn't exist or is not a directory
      /Users/hina.khadim/node_modules doesn't exist or is not a directory
      /Users/node_modules doesn't exist or is not a directory
      /node_modules doesn't exist or is not a directory
 @ ./node_modules/@edx/frontend-platform/initialize.js 437:0-35 570:23-32 575:17-26 581:19-28
 @ ./node_modules/@edx/frontend-platform/index.js 3:0-68 3:0-68 3:0-68 3:0-68 3:0-68
 @ ./src/index.jsx 10:0-103 14:0-9 14:10-19 21:0-9 21:10-24 27:0-10 30:6-17

webpack 5.94.0 compiled with 1 error in 514 ms

Solution:

const FooterModule = await import(/* webpackIgnore: true */`@edly-io/indigo-frontend-component-footer`).catch(() => null); // Fallback to null if not available

I matched the frontend-platform and plugin-framework versions with other MFEs but that didn't helped. Don't know the exact reason of this issue but webpackIgnore helped

@hinakhadim
Copy link
Contributor

hinakhadim commented Dec 3, 2024

Hi @arbrandes ,

There's an issue generating with this PR on tutor-indigo that is it's not picking up by any MFE. The image building is successful and when we try to see in browser. The changes are not appearing. Steps:

  • install indigo footer in leaner-dashboard MFE
  • the contents of env.config.jsx file:
import React, { useEffect } from 'react';
import Cookies from 'universal-cookie';

import { getConfig } from '@edx/frontend-platform';

function addPlugins(config, slot_name, plugins) {
  // ...
}

let themeCookie = 'indigo-toggle-dark';
let themeCookieExpiry = 90; // days

const AddDarkTheme = () => {
  // ...

  return (<div />);
};

async function setConfig () {
  let config = {
    pluginSlots: {}
  };

  try {
    /* We can't assume FPF exists, as it's not declared as a dependency in all
     * MFEs, so we import it dynamically. In addition, for dynamic imports to
     * work with Webpack all of the code that actually uses the imported module
     * needs to be inside the `try{}` block.
     */
    const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework');

const Footer = await import(/* webpackIgnore: true */`@edly-io/indigo-frontend-component-footer`).catch(() => null); // Fallback to null if not available

    if (process.env.APP_ID == 'authn') {
    }
    if (process.env.APP_ID == 'authoring') {
    }
    
    if (process.env.APP_ID == 'learner-dashboard') {
      addPlugins(config, 'footer_slot', [ 
            {
                op: PLUGIN_OPERATIONS.Hide,
                widgetId: 'default_contents',
            },
            {
                op: PLUGIN_OPERATIONS.Insert,
                widget: {
                    id: 'default_contents',
                    type: DIRECT_PLUGIN,
                    priority: 1,
                    RenderWidget: Footer.default,
                },
            },
            {
            op: PLUGIN_OPERATIONS.Insert,
                widget: {
                    id: 'read_theme_cookie',
                    type: DIRECT_PLUGIN,
                    priority: 2,
                    RenderWidget: AddDarkTheme,
                },
            },
  ]);
    }
   
  } catch { }

  return config;
}

export default setConfig;
  • I was thinking that AddDarkTheme func is not working. But the default footer is not hidden and new Indigo footer is not applied.
  • I added the previous env.config.jsx that exists in Indigo. and that works perfectly.

Reason of Issue:
I'm assuming that this is causing because of setConfig. export default setConfig is a function and in the previous env.config.jsx (already used in indigo now), we are exporting object let config = {...}; export default config; (object).

Could you please help in resolving it? I tried to figure it out by calling setConfig() but it didn't work. Because it's also async function and return Promise. Also, if you help how to add logger to env.config.jsx, then it helps in debugging it easily.

@arbrandes
Copy link
Collaborator Author

@hinakhadim, lemme try and reproduce it on my end. Is there a PR to tutor-indigo that I can refer to?

@hinakhadim
Copy link
Contributor

Sure, please. Here's the PR Link overhangio/tutor-indigo#109

It might be webpack Ignore is causing this issue because I remember it was working before. But the problem is ora-grading fails the build if I don't import Footer with webpack Ignore. (FYI: I also tried by importing Footer in if conditions. And one thing is that we aren't included footer in ora-grading but still I don't have idea why it's reading the Footer import during build and produces error).

Error is in this conversation: #240 (comment) . At that time my env.config.jsx was like: (As you can see, footer in even imported in learner-dashboard condition. It has nothing with ora-grading, but still failed the build.

...
try{

const { DIRECT_PLUGIN, PLUGIN_OPERATIONS } = await import('@openedx/frontend-plugin-framework');
 
     if (process.env.APP_ID == 'authn') {
    }
    ...
    if (process.env.APP_ID == 'learner-dashboard') {
        const Footer = await import("@edly-io/indigo-frontend-component-footer");
         
        addPluginSlot(...);
    }
   ...
   if (process.env.APP_ID == 'ora-grading') {
    }
    ...
} catch {}
...

After adding webpack Ignore , image building is becoming successful but MFEs are not picking this file or may be picking but not applying any plugin slot.

@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch from 7daaeb5 to 7e34b50 Compare December 5, 2024 14:31
@arbrandes
Copy link
Collaborator Author

@hinakhadim, I have some findings to discuss, but I'll do it as a review to overhangio/tutor-indigo#109.

@arbrandes
Copy link
Collaborator Author

@hinakhadim, now that the MFEs have been fixed, let me know if things are good with overhangio/tutor-indigo#109 so we can merge this.

@hinakhadim
Copy link
Contributor

Things are working good. Just waiting for a heads-up from another member regarding testing before we proceed with the merge. Status: overhangio/tutor-indigo#109 (comment)

@DawoudSheraz
Copy link
Contributor

@arbrandes Hi. Please fix the failing tests so that this can be merged. Thanks

@arbrandes
Copy link
Collaborator Author

@DawoudSheraz, I'm looking into it, but to be honest, it'll probably take me a while. My expertise with Python type annotations is limited.

@arbrandes
Copy link
Collaborator Author

Plus, I'm not currently set up to reproduce the test environment. 😓

"""
get_mfes.cache_clear()
i: tuple[str, str, str]
return [i[-2:] for i in PLUGIN_SLOTS.iterate() if i[0] == mfe_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

iterate can only be called on filters that return a list of things. I think that the annotation of PLUGIN_SLOTS is incorrect. It should be:

PLUGIN_SLOTS: Filter[list[tuple[str, str, str]], []]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch! This solved the linting cascade.

"""
get_mfes.cache_clear()
i: tuple[str, str, str]
Copy link
Contributor

Choose a reason for hiding this comment

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

This annotation should not be necessary, let's get rid of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this was just a dumb attempt to get the linting to stop complaining before I got python 3.9 set up locally. 😅

@hinakhadim
Copy link
Contributor

For the issue at line 124, I'm not entirely sure if this is the most appropriate solution, but it does resolve the problem

def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE:
    mfes : dict[str, MFE_ATTRS_TYPE] = get_mfes() 
    return mfes.get(mfe_name, {})

@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch 2 times, most recently from 198e3ea to 93c21cb Compare December 9, 2024 17:56
def is_mfe_enabled(mfe_name: str) -> bool:
return mfe_name in get_mfes()


def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE:
def get_mfe(mfe_name: str) -> t.Union[MFE_ATTRS_TYPE, t.Any]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hinakhadim, your previous comment got me on the right track, thanks - but I didn't want to change running code just for the sake of a type hint, so this is what I did. It seems there's significant precedent for Any in the codebase, so I expect it to be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is caused by the lru_cache decorator, which is not correctly typed. You can bypass this issue without Any by writing:

def get_mfe(mfe_name: str) -> MFE_ATTRS_TYPE:
    mfe: MFE_ATTRS_TYPE = get_mfes().get(mfe_name, {})
    return mfe

This provides a mechanism to configure frontend plugins using a new
PLUGIN_SLOTS filter and a series of patches that modify a base
`env.config.jsx`, in such a way that multiple plugins can take advantage
of the file (including for purposes beyond frontend plugins) without
clobbering each other.
@arbrandes arbrandes force-pushed the frontend-plugin-support-take-2 branch from 93c21cb to 69f3467 Compare December 9, 2024 18:03
@arbrandes
Copy link
Collaborator Author

Ok, finally got pyenv properly set up so I could reproduce the failures. Thanks for the reviews!

@DawoudSheraz DawoudSheraz merged commit 00cb277 into overhangio:main Dec 10, 2024
2 checks passed
@arbrandes arbrandes deleted the frontend-plugin-support-take-2 branch December 11, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants