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

Cleanup require import #2139

Merged
merged 10 commits into from
Oct 24, 2023
Merged

Cleanup require import #2139

merged 10 commits into from
Oct 24, 2023

Conversation

omsaggau
Copy link
Contributor

No description provided.

@annesiri
Copy link
Contributor

Generelt ser jeg at det nå ligger en del tomme linjer inne mellom importene, og en del steder er det ikke linjeskift etter importer og før implementasjonen starter. Jeg kunne ønsket meg at det var lettere å kjapt se hva som hører sammen, men det er kanskje ikke en del av denne oppgaven? Eller er det noe eslint/prettier kan fikse for oss det også?

@johnnadeluy
Copy link
Contributor

johnnadeluy commented Oct 13, 2023

So far, I've identified a few "util is not defined" errors:

ReferenceError: "util" is not defined
org.openjdk.nashorn.internal.runtime.ECMAException: ReferenceError: "util" is not defined

Can be found in:

  • Dashboard, "Kommende publiseringer" and "Spørringer" -> "Andre". When manually updating fetch statreg statistics under "Spørringer".
  • Best-bet.
  • Custom selectors have "Service error: Unknown error" due to the same util error.
  • Highcharts.
  • Possible other places as well?

@omsaggau
Copy link
Contributor Author

So far, I've identified a few "util is not defined" errors:

ReferenceError: "util" is not defined
org.openjdk.nashorn.internal.runtime.ECMAException: ReferenceError: "util" is not defined

Can be found in:

  • Dashboard, "Kommende publiseringer" and "Spørringer" -> "Andre". When manually updating fetch statreg statistics under "Spørringer".
  • Best-bet.
  • Custom selectors have "Service error: Unknown error" due to the same util error.
  • Highcharts.
  • Possible other places as well?

Resolved. Alle these placese used arrayUtils which was where the bug was.

@@ -42,7 +44,7 @@ function renderPart(req: XP.Request): XP.Response {

// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
Copy link
Contributor

Choose a reason for hiding this comment

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

Tror ikke vi lenger trenger eslint-unntakene etter at du har ryddet (linje 40, 45 og 46).

const mode: string = pageMode(req)
if (!municipality && mode === 'edit') {
const siteConfig = getSiteConfig<XP.SiteConfig>()
if (!siteConfig) throw Error('No site config found')

municipality = getMunicipality({
code: siteConfig.defaultMunicipality,
} as unknown as XP.Request)
} as unknown as RequestWithCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Skjønner ikke hvorfor vi går via unknown her? Gjør ikke det implisitt at vi mister typesjekken? (Samme kommentar gjelder for linje 32). Er vel kanskje litt på siden av oppgaven, men siden vi likevel er inne og fikser typen...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ser ut som det ligger inne et es-lint-unntak på linje 102 som ikke lenger er nødvendig.


export function get(req: XP.Request) {
const config = getComponent<XP.PartComponent.MailChimpForm>()?.config
if (!config) throw Error('No part found')

const phrases: Phrases = getPhrases(getContent()) as Phrases
const currentContent = getContent() ?? ({} as Content)
const phrases: Phrases = getPhrases(currentContent) as Phrases
Copy link
Contributor

Choose a reason for hiding this comment

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

Phrases er typet dobbelt. Siden det er satt as Phrases trengs vel ikke : Phrases også?

const language: Language = getLanguage(page)
const phrases: Phrases = language.phrases as Phrases
const language = getLanguage(page)
const phrases: Phrases = language?.phrases as Phrases
Copy link
Contributor

Choose a reason for hiding this comment

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

Her er også typen til phrases dobbelt definert (se forøvrig forrige kommentar)

import { Content } from '/lib/xp/content'
import type { Highchart } from '/site/content-types'
import { RowValue } from '/lib/ssb/utils/utils'
import { RowValue, getRowValue } from '/lib/ssb/utils/utils'
import { toString } from '/lib/vendor/ramda'
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

fjern ts-ignore her

import { DatasetRepoNode } from '/lib/ssb/repo/dataset'
import { datasetOrUndefined } from '/lib/ssb/cache/cache'
import { type CalculatorConfig, type GenericDataImport } from '/site/content-types'
/* eslint-disable new-cap */
Copy link
Contributor

Choose a reason for hiding this comment

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

Ser ut som disse to linjene med unntak henger igjen og ikke trengs lenger??

Copy link
Contributor

Choose a reason for hiding this comment

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

ts-ignore på linje 110 tror jeg kan fjernes nå

Copy link
Contributor

Choose a reason for hiding this comment

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

Ser ut som ts-ignore på linje 32 kan fjernes.

@johnnadeluy
Copy link
Contributor

Highcharts with excel as their data source get the following error: TypeError: _striptags is not a function

const language: Language = getLanguage(page)
const phrases: Phrases = language.phrases as Phrases
const language = getLanguage(page)
const phrases: Phrases = language?.phrases as Phrases
Copy link
Contributor

Choose a reason for hiding this comment

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

Phrases is typed twice

@omsaggau omsaggau changed the base branch from master to develop October 24, 2023 08:19
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

@omsaggau omsaggau merged commit cd4c2a4 into develop Oct 24, 2023
3 of 4 checks passed
@omsaggau omsaggau deleted the cleanup-require-import branch October 24, 2023 08:40
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