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

Update eslint plugins, heed its warnings; mostly about structuredCloned (TIL) #6483

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mattgodbolt
Copy link
Member

Also contains:

  • String.raw (TIL) - in since node.js 4.x
  • Typing fixes and ...result fixes in arg parsers, bug perpetrated by yours truly.

@mattgodbolt mattgodbolt requested a review from partouf May 14, 2024 03:38
@mattgodbolt
Copy link
Member Author

Well, Cypress is unhappy with:

    } could not be cloned. {"stack":"DataCloneError: function (property, defaultValue) {\n        return get(base, property, defaultValue);\n    } could not be cloned.\n    at new DOMException (node:internal/per_context/domexception:53:5)\n    at structuredClone (node:internal/structured_clone:23:17)\n    at ClientOptionsHandler.setCompilers (file:///home/runner/work/compiler-explorer/compiler-explorer/lib/options-handler.ts:308:33)\n    at onCompilerChange (file:///home/runner/work/compiler-explorer/compiler-explorer/app.ts:501:36)\n    at main (file:///home/runner/work/compiler-explorer/compiler-explorer/app.ts:506:11)"}

@mattgodbolt
Copy link
Member Author

so we probably have been json to/fromming all manner of junk that doesn't rounttrip through json since forever

@mattgodbolt mattgodbolt marked this pull request as draft May 14, 2024 03:41
@partouf
Copy link
Contributor

partouf commented May 14, 2024

So in a couple of these a function gets stored in CompilerInfo https://github.com/compiler-explorer/compiler-explorer/blob/main/lib/compiler-finder.ts#L330

This is basically a way to get special property settings specific to this context without having to store things in compiler-finder.ts (e.g. https://github.com/compiler-explorer/compiler-explorer/blob/main/lib/buildenvsetup/ceconan.ts#L62)

But of course this causes issues when using structuredClone()

The function getting lost during copy isn't a problem, because at that point the compilers have already been instantiated including buildenvsetup. That's why parse(stringify()) wasn't a problem. (although the function gets manually reintroduced when reading a discovery json)

We can probably just use the function directly instead of storing it. Gonna give that a try.

@partouf
Copy link
Contributor

partouf commented May 14, 2024

Those are fixed and simplified.

But the Tools remain, they are classes (they get replaced again later)

Bit too complicated to fix that at the moment.

Basically I'm still reverting the change in options-handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants