Point in time merge: CLI and Header work#35
Merged
Conversation
Will be adding other things to it shortly.
Other new helpers will follow.
The preset code has always been kind of difficult to reason. This simplifies the data structures and makes them more approachable. There’s now a getResolvedConfigPath function which determines which config file to use for any given config type; all the logic is co-located in that file, and there’s a simple `defaultConfigPaths` data structure that holds all our ‘fallback’ config file locations. Further, all paths are resolved and verified before use, and the build is aborted if the resolved file isn’t actually present.
This will be used in the configs shortly.
Prior to this function, site config files were hard coded into the webpack configs. This allows them to be defaulted and overridden via an environment variable supplied on the command line: SITE_CONFIG_PATH. The SITE_CONFIG_PATH can be relative to the current working directory (process.cwd()) or an absolute path.
This adds a ‘common-config’ folder under tools/webpack where we can extract config that’s common to one or more webpack configs. There’s a README that describes the spirit of this folder - to summarize: don’t over-use it. Subsequent commits will add other extracted pieces of config.
It’s identical in all webpack configs.
getParagonEntryPoints has the exact same comments in it; let’s just have those.
Still need to get it used for ts-loader, which will happen in a subsequent commit. The ts-loader config is being extracted to a separate file.
Noting in particular that uniqueName is necessary for module federation to function.
Don’t think these comments about the differences between dev/prod builds are very helpful.
This was necessary when the devServer config was in the dev files - oddly, it was also in the ‘build’ configs which makes no sense.
They’re identical in all webpack configs.
Prior commits will break without this, as we’re importing them into the webpack configs from this file. Oops.
This is admittedly a bit of an experiment, as we were using ‘auto’ in some places and ‘/‘ as the default in others. I _believe_ that ‘auto’ should be fine everywhere unless one has been specified. If not, then this may have to be backed out or modified.
As part of this, I’m assuming that the ‘oneOf’ split in the dev config is also good for the dev.module config. If something about that doesn’t work, this may need to be more conditional.
This also cleans up the imports in our webpack configs, and is the last major refactor. Onward!
Needed to be updated to work with application module configs. Also fixed a bug in getPublicPath() where we were setting ‘auto’ for the path in the devServer, resulting in a path of “auto/index.html” for the index file.
This lets us get rid of example-plugin-app, which contained two iframe pages, only one of which was actually used.
It’s needed because the stylesheet rule now uses it for this webpack config.
SiteConfig is the internal type.
… webpack configs Adds type checking to webpack’s build, and helps react refresh work with TS. I think.
It wasn’t being used, and was confusing.
It should use it when they’re an array, not the other way around.
We may need to further differentate things referred to ‘module configs’ but this is a good start.
The new header is flexible enough to do the jobs of all three existing headers. This commit just cleans out the old to make way for the new.
This is a first stab at a header configurable enough to support the proposal for a customizable header for the LMS. https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4412243977/Proposal+Customizable+Header+for+LMS There’s still work to do, but at present, it supports everything the original three headers supported, but in a configurable way (via site config).
This means each app (module) in the configuration will be keyed by its app ID, which makes it easier to look up app paths by their ID. i.e., we no longer have to iterate over the list of apps. This is in preparation for a new function that will return the path or URL to an app based on its app ID.
This is a core piece of functionality for the shell, which will allow any app to get a URL to any other based on the site configuration. Depending on how the module is being loaded, this could be a path within the single page app or a fully qualified URL to an external site.
The header can now be configured from the site config, either at a global or app-specific level. The logo URLs, primary links, secondary links, authenticated dropdown links, and anonymous links can all be configured in a variety of ways, including an ‘escape hatch’ of an arbitrary React element. There’s still some work to do on the types to make them understandable - the words ‘config’, ‘app’, and ‘module’ are overused and need differentiation. The header provides a context of its resolved configuration to all its children, so any plugins or override components will have access to the complete config. The “CourseInfo” component is still a stub that needs to be fleshed out further.
The code that adds the appId into the route.handle object is going to be re-used by the federated module code. This pulls it out into a function.
Things that I wish Paragon had.
Moving toward a single messages file for the entire Shell.
This feels more canonical than using width: 100% on three elements and letting them fight for space.
There should be no way they are undefined, since they’re defaulted.
Seems like an obvious oversight. Oops.
This determiens if an app exists and if configurable, meaning it’s one of an INTERNAL or FEDERATED app.
In that all the properties MUST be defined in the base.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @davidjoy! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR spans work from the past few months. It includes improvements to the
openedxCLI tool, as well as a new, configurable Header component to replace the headers from frontend-component-header, which have been deleted.