-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Fix global typings for extensions #2992
Conversation
4fe4202
to
4879389
Compare
4879389
to
e0d53bc
Compare
bebc194
to
7af25ae
Compare
98576b5
to
b712b4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested locally, but generally LGTM. Solid set of changes!
Left a few nits.
Co-authored-by: Alexander Skvortsov <sasha.skvortsov109@gmail.com>
Weird 🤔, compiling with this seems to break extensions that use TS but don't import Uncaught TypeError: Cannot read property 'addTranslations' of undefined
Uncaught TypeError: flarum.core.app.load is not a function |
Hmm... Typing changes couldn't do that as that's handled by the webpack config... Which extension is this? |
never mind, it doesn't have anything to do with extensions, just checking out this branch and building dist files results in the error above. could it be the |
It doesn't. In my testing, returning My thinking was that the app instances are created by the forum/admin exports, and overwriting them with a Common app would be disastrous. |
578b46a
to
c1e2c91
Compare
Fixes #2963
Changes proposed in this pull request:
app
typings (people should import instead)app
exportapp
imports to coreflarum
variableReviewers should focus on:
To test this locally...
composer.json
composer update
cd vendor/flarum/core/js
npm i
npm run build-typings
tsconfig.json
as per Add configuration for global typings flarum-tsconfig#2Screenshot
Confirmed
composer test
).Required changes: