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

Tsify main #4520

Merged
merged 8 commits into from
Jan 14, 2023
Merged

Tsify main #4520

merged 8 commits into from
Jan 14, 2023

Conversation

jeremy-rifkin
Copy link
Member

@jeremy-rifkin jeremy-rifkin commented Dec 30, 2022

(Also fixes #4521)

@github-actions github-actions bot added the ui label Dec 30, 2022
@@ -0,0 +1,34 @@
// Copyright (c) 2022, Compiler Explorer Authors
Copy link
Member Author

Choose a reason for hiding this comment

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

File is from #4490

@@ -195,7 +195,6 @@ export class Editor extends MonacoPane<monaco.editor.IStandaloneCodeEditor, Edit
override createEditor(editorRoot: HTMLElement): editor.IStandaloneCodeEditor {
const editor = monaco.editor.create(
editorRoot,
// @ts-expect-error: options.readOnly and anything inside window.compilerExplorerOptions is unknown
Copy link
Member Author

Choose a reason for hiding this comment

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

Got an error TS2578: Unused '@ts-expect-error' directive here, I went ahead and added readOnly to options.interfaces.ts while I was here

static/main.ts Outdated Show resolved Hide resolved
static/main.ts Outdated Show resolved Hide resolved
static/main.ts Outdated Show resolved Hide resolved

const siteTemplateScreenshots = require.context('../views/resources/template_screenshots', false, /\.png$/);

if (!window.PRODUCTION && !options.embedded) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes #4521, cc @RubenRBS It looks like global.sinon isn't defined for embedded in static/tests/motd

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, good catch

@jeremy-rifkin jeremy-rifkin marked this pull request as ready for review December 31, 2022 06:35
Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Also love this one :)
You'll need to deal with the duplicated file in both types PRs, but nothing too hard specially as both PRs are yours

@@ -22,42 +22,52 @@
// ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
// POSSIBILITY OF SUCH DAMAGE.

'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there were no type errors in the js version judging by the diff, which is pretty unexpected seeing how the rest of the TS conversions have gone down, so I'm pleasantly supprised :)

function fixBugsInConfig(config) {
if (config.activeItemIndex && config.activeItemIndex >= config.content.length) {
config.activeItemIndex = config.content.length - 1;
// TODO(jeremy-rifkin): Unsure of the type, just typing enough for `content` at the moment
Copy link
Member

Choose a reason for hiding this comment

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

I'm suprised that this is not just a config from GoldenLayout 🤔 That's weird

Copy link
Member

Choose a reason for hiding this comment

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

I think it is the golden layout config yes...which we're trying to fix up to account for a short lived bug from the early 17th Century where the item index count be longer than the content.

That said: I can't see any evidence for that in the git history: @partouf appears to have added this in 5e5e601 and it doesn't look like it was there before (or my git-fu is weak, which is very possible)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, I just tried typing as GoldenLayout.Config but apparently config.activeItemIndex doesn't exist

.object()
.value();
const filters = Object.fromEntries(((params.filters as string) || '').split(',').map(o => [o, true]));
// TODO(jeremy-rifkin): Fix types
Copy link
Member

Choose a reason for hiding this comment

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

What do you think is the easiest way to do this in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember why I put this comment here :P I think it was just about the various casts. The string cast should be resolved once we type params and then I think a small modification of the ParseFiltersAndOutputOptions type will "just work"

Copy link
Member

@mattgodbolt mattgodbolt left a comment

Choose a reason for hiding this comment

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

LGTM: one superminor only, feel free to merge (but let me know when you do!) I'd like to do a "zomg typescript" push this month and this is a key part :)

@@ -42,7 +44,7 @@ export interface SiteSettings {
colourScheme: ColourScheme;
compileOnChange: boolean;
// TODO(supergrecko): make this more precise
Copy link
Member

Choose a reason for hiding this comment

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

is this TODO done now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think so!

function fixBugsInConfig(config) {
if (config.activeItemIndex && config.activeItemIndex >= config.content.length) {
config.activeItemIndex = config.content.length - 1;
// TODO(jeremy-rifkin): Unsure of the type, just typing enough for `content` at the moment
Copy link
Member

Choose a reason for hiding this comment

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

I think it is the golden layout config yes...which we're trying to fix up to account for a short lived bug from the early 17th Century where the item index count be longer than the content.

That said: I can't see any evidence for that in the git history: @partouf appears to have added this in 5e5e601 and it doesn't look like it was there before (or my git-fu is weak, which is very possible)

@@ -867,7 +867,7 @@ export class Editor extends MonacoPane<monaco.editor.IStandaloneCodeEditor, Edit

changeLanguage(newLang: string): void {
if (newLang === 'cmake') {
this.selectize.addOption(languages.cmake);
this.selectize.addOption(unwrap(languages.cmake));
Copy link
Member Author

Choose a reason for hiding this comment

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

Merging with main introduced a type error here so I needed to unwrap

@jeremy-rifkin jeremy-rifkin merged commit 33167a3 into main Jan 14, 2023
@jeremy-rifkin jeremy-rifkin deleted the jr/tsify-main branch January 14, 2023 00:52
mattgodbolt pushed a commit that referenced this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed urls are broken locally
3 participants