feat: merging ongoing work upstream #37
Closed
Conversation
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
There were several breaking changes, new warnings, and changes in here. - Migrate from @import to @use in SCSS files. - replace “compileType” with “mode” in css-loader options - Set “namedExport” to false to preserve behavior of earlier css-loader versions - Switching from ‘sass’ to ‘sass-embedded’ to improve build performance.
This commit upgrades us to the latest ESLint 9. It adds better linting for TypeScript, in particular. It removes eslint-plugin-airbnb and replaces it with the ‘recommended’ configs for eslint, typescript-eslint, react, and react-hooks. It also adds the eslint stylistic config, and modifies it to match our existing code style. Subsequent commits will fix all the styling issues found in this commit; the repository wasn’t linting very consistently before this, so somehow a bunch of stuff slipped through.
I had a checkout with a ton of linting changes and just overlooked these files.
Because of the new config, a ton of eslint ‘ignore’ comments and such are no longer necessary.
Also getting rid of some unneeded eslint disable line comments
Also using ‘exec’ on a Regex instead of match.
The existing createConfig helper is not appropriate for the new version of ESLint for two reasons - one, it uses webpack-merge, while we need to use tseslint.config to merge config. Second, the argument of a single “configFragment” object is not what tseslint needs - it needs an array of ConfigWithExtends objects. For these reasons, I just split the lint config helper out into a separate function.
PluralRules and RelativeTimeFormat are widely available in browsers today, so these are no longer necessary.
Modules that need these functions should implement them themselves - frontend-app-account in particular. We don’t want to load these lists for the vast majority of the frontend that doesn’t need them. This also removes the third party dependencies that backed the lists.
It was duplicated in both the optional and required configs. Oops.
Our route objects have an optional “role” in the handle. This change to the types will allow that role to show up in auto-complete if a handle is added, along with a helpful comment explaining what it’s for.
The configure functions for auth, analytics, logging, and i18n are now named explicitly for the service they configure: - configureAuth - configureAnalytics - configureI18n - configureLogging The public API has already been changed, this just changes the internals to make it all easier to parse and follow through the code.
Makes it simpler to maintain our export lists.
- organizing ‘routing’ folder into utils file - fixing getUrlByRouteRole naming - adding isRoleRouteObject helper
A ‘role’ is a semantically meaningful identifer for a route or widget that fulfills a particular job in the site. Like the ‘login page’, ‘studio’, or ‘header’. We use roles to know what’s being displayed in the UI at any given time, particularly for UI Operation conditions. A UI operation can now use an “active” condition to perform an operation only if a particular role or set of roles is active.
Still need to make a proper test setup for the iframe widget.
It’s an app, not a config.
Nothing needs these at the moment, but they’ll likely be helpful and mirror other helpers that are already in the file.
For simplicity’s sake, we let the type of the layout prop be ReactNode and then weed out any sub-types that aren’t an actual layout component, i.e., boolean, null, and undefined.
…NTICATED_USER_CHANGED The useOperations hook can use getSlotOperations to set its value immediate without waiting for useEffect to fire. It also should NOT listen to AUTHENTICATED_USER_CHANGED; the user’s auth state changing does not affect the unfiltered list of operations for a slot, so listening for it would cause unnecessary re-renders.
Apps generally have custom config values that they need to supply on load; this allows them to supply that config as part of the App, and the Shell will make it available through the getAppConfig(id: string) function after processing it. This helps differentiate between the site’s core config available through getConfig and App-specific config now available through getAppConfig.
This is temporary, and just to show how it works.
The useActiveRouteRoleWatcher hook needs to exist inside a router, which isn’t always true for tests. This is functionally equivalent, but means test suites throughout the platform will not have to change. Also moved usTrackColorSchemeChoice too, which leaves AppProvider focused on its contexts.
Apparently you don’t import types used in the declaration? Wild.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @davidjoy! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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. DetailsWhere 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. |
Contributor
Author
|
@arbrandes @brian-smith-tcril This branch should really be merged, FYI. It's a ton of upstream work, very important. Let me know if you want me to just do it. |
Contributor
|
@davidjoy, we'll definitely merge it! I was just hoping to get the tests passing first, but haven't gotten around to it, yet. |
Contributor
|
Merged via #39. |
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 merges all my current work from the past few months into frontend-base. The code has changed significantly, with a re-worked "App" model containing routes, messages, slots, config, and remotes. It also includes a replacement for FPF which has been unified with the app module system.