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

Fixing a build error introduced by quicktype enums #1090

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Oct 17, 2023

THe latest version of quicktype has started generating single value enums for constant values, which are causing issues in the context type tests (Context types serialize but cause type errors on deserialization). However, its also capable of generating const values - although this option hasn't been exposed correctly through the CLI (raising a PR to fix). Generating true const value fixes the issue.

I hadn't noticed this test failing when I merged:

Also resolving a critical vulnerability (babel) via npm audit fix

@netlify
Copy link

netlify bot commented Oct 17, 2023

Deploy Preview for fdc3 canceled.

Name Link
🔨 Latest commit 7ac81ab
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/652e7e31ea9dfa0009f6915c

@kriswest kriswest changed the title Fixing a build error introduced by quicktype enums, Fixing a build error introduced by quicktype enums Oct 17, 2023
@kriswest
Copy link
Contributor Author

Apologies @hughtroeger @mattjamieson, looks like the last PR I merged broke the test (npm run test) this fixes them again.

@kriswest
Copy link
Contributor Author

QuickType PR for exposing the prefer-const-values setting: glideapps/quicktype#2426

To use a custom quicktype change s2tQuickTypeUtil.js to point quicktypeExec at the local version:

// Normalise path to local quicktype executable.
const quicktypeExec = "node " + ["..","quicktype","dist","index.js"].join(path.sep);
//const quicktypeExec = ['.', 'node_modules', '.bin', 'quicktype'].join(path.sep);

const command = `${quicktypeExec} --prefer-const-values --no-combine-classes -s schema -o ${outputFile} ${sources}`;

Current version:

FDC3/s2tQuicktypeUtil.js

Lines 43 to 46 in d127364

// Normalise path to local quicktype executable.
const quicktypeExec = ['.', 'node_modules', '.bin', 'quicktype'].join(path.sep);
const command = `${quicktypeExec} --no-combine-classes -s schema -o ${outputFile} ${sources}`;

@kriswest
Copy link
Contributor Author

kriswest commented Oct 17, 2023

This has not fixed the issue as the package scripts are regenerating the context types during the build step (by automatically calling preprepare). We'll either need to disable that or wait until Quicktype merges the option to expose the --prefer-const-values setting...

@kriswest
Copy link
Contributor Author

Ok, its passing now - as long as the types are not regenerated. Here's hoping quicktype merge that PR soon

Copy link
Contributor

@hughtroeger hughtroeger left a comment

Choose a reason for hiding this comment

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

Actually, why is there a package-lock.json change when the package.json doesn't have any changed dependencies? Is that intentional?

@kriswest
Copy link
Contributor Author

Actually, why is there a package-lock.json change when the package.json doesn't have any changed dependencies? Is that intentional?

npm audit fix did that. Its basically the same set of changes to resolve a critical vulnerability that dependabot wants to make in this PR: #1089
I'm not sure why it overrides the resolutions in the lock file rather than updating the package.json - but we can do that manually...

@kriswest
Copy link
Contributor Author

@hughtroeger its because is a transitive dependency, i.e. its coming in through tsdx, husky and jest:

PS C:\Users\Kris\Documents\code\FDC3> npm ls @babel/code-frame
@finos/fdc3@2.1.0-beta.3 C:\Users\Kris\Documents\code\FDC3
├─┬ husky@4.3.8
│ └─┬ cosmiconfig@7.1.0
│   └─┬ parse-json@5.2.0
│     └── @babel/code-frame@7.22.13 deduped
├─┬ jest-mock-extended@1.0.18
│ └─┬ jest@25.5.4
│   └─┬ @jest/core@25.5.4
│     └─┬ jest-message-util@25.5.0
│       └── @babel/code-frame@7.22.13 deduped
└─┬ tsdx@0.14.1
  ├─┬ @babel/core@7.21.4
  │ ├── @babel/code-frame@7.22.13
  │ └─┬ @babel/template@7.22.15
  │   └── @babel/code-frame@7.22.13 deduped
  ├─┬ @babel/traverse@7.23.2
  │ └── @babel/code-frame@7.22.13 deduped
  ├─┬ babel-eslint@10.1.0
  │ └── @babel/code-frame@7.22.13 deduped
  ├─┬ eslint@6.8.0
  │ └── @babel/code-frame@7.22.13 deduped
  └─┬ rollup-plugin-terser@5.3.1
    └── @babel/code-frame@7.22.13 deduped

It'll get sorted properly when we replace tsdx with something better maintained (see #1061)

@hughtroeger
Copy link
Contributor

Ahh ok that makes sense. Looks good to me then.

Copy link
Contributor

@mattjamieson mattjamieson left a comment

Choose a reason for hiding this comment

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

LGTM!

@kriswest kriswest merged commit a656549 into master Oct 18, 2023
10 checks passed
@kriswest kriswest deleted the fix-build-error branch October 18, 2023 13:10
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.

3 participants