Skip to content

fix(project): keep non-ASCII characters in slugified identifiers - #1531

Open
CaptainAni187 wants to merge 1 commit into
OpenFn:mainfrom
CaptainAni187:fix/slugify-unicode
Open

CaptainAni187 wants to merge 1 commit into
OpenFn:mainfrom
CaptainAni187:fix/slugify-unicode

Conversation

@CaptainAni187

Copy link
Copy Markdown

Closes #1388

slugify() used text.replace(/\W/g, ' '). JavaScript's \W is ASCII-only, so every non-Latin character was treated as punctuation:

input before after
café caf café
résumé r-sum résumé
Vérifier l'état du patient v-rifier-l-tat-du-patient vérifier-l-état-du-patient
患者確認 `` (empty) 患者確認
проверка пациента `` (empty) проверка-пациента

As #1388 points out, these slugs are identity rather than display — they set the workflow id, the job id and the keys used for edge references — so the pull/deploy round-trip currently loses information for anyone naming steps outside ASCII.

Change

Match letters and digits with \p{L} and \p{N} under the u flag instead of \W. Underscore is kept explicitly, since \W treated it as a word character too.

Existing identifiers do not change. For ASCII-only input the new expression produces exactly the same slug as the old one — I checked every printable-ASCII pair plus 200,000 random ASCII strings (209,025 inputs) and found zero differences. That matters here because a change in output would silently re-key workflows in existing projects.

Emoji and punctuation still become separators, so deploy 🚀 now is still deploy-now.

On the library suggestion

@josephjclark suggested reaching for a library like slugify in the issue thread. I went with the Unicode-property regex instead because the popular libraries transliterate — slugify('café') gives cafe, and slugify('патент') gives patent. That conflicts with the point made in the same thread, that it's good for the user's identifier to survive without conversion, and it wouldn't help a script with no Latin transliteration at all. Happy to switch to a library if you'd rather have transliteration; it's a one-line swap plus a dependency.

I have not tried to match the Apollo rules @hanna-paasivirta linked (OpenFn/apollo#446) — that looked like a separate alignment question, and I did not want to widen this beyond the reported bug.

tsconfig

Unicode property escapes require an ES2018 target and tsconfig.common sets no target, so tsc defaulted to ES5 and rejected the u flag with TS1501. This sets "target": "ES2020" on the project package only. That file is validation-only — the comment at the top of tsconfig.common notes build artefacts come from tsup — so runtime output is unaffected. If you would rather not touch tsconfig, building the regex through new RegExp(..., 'gu') sidesteps the check, but that seemed worse than declaring the real requirement.

Tests

Adds test/util/slugify.test.ts (9 tests): existing ASCII behaviour, underscores and digits, separator collapsing and trimming, accented Latin, three non-Latin scripts, emoji still dropped, and empty/nullish input.

  • packages/project suite: 252 passed, up from 243, with the same 4 skipped, 1 todo and 7 uncaught exceptions before and after — those are the pre-existing mock-fs problems described in replace mock-fs in unit tests #1346.
  • tsc --noEmit: the TS1501 error is gone; the three remaining Cannot find module '@openfn/logger' errors are present on a clean checkout too (unbuilt workspace dependency).

`slugify()` replaced every `\W` character with a separator. JavaScript's `\W`
is ASCII-only, so any non-Latin character was treated as punctuation:
`café` became `caf`, `résumé` became `r-sum`, and a name written entirely in
a non-Latin script collapsed to an empty string.

These slugs are identity, not display: they set workflow and job ids and the
keys used for edge references, so the CLI's pull/deploy round-trip loses
information for anyone naming steps outside ASCII.

Match letters and digits with the Unicode properties `\p{L}` and `\p{N}`
instead. Underscore is still kept, matching the old `\W` behaviour, and for
ASCII-only input the output is unchanged.

Unicode property escapes need an ES2018 target, so this sets `target` on the
project package's tsconfig. Build output comes from tsup; that file only
drives type validation.
@CaptainAni187

Copy link
Copy Markdown
Author

Note on the red integration_test job — it doesn't look related to this change.

The worker integration suite hangs rather than fails:

48 tests passed
4 tests skipped
3 tests remained pending after a timeout
Failed

Nothing errors; three tests just never resolve and ava times out. That's the same open-handle symptom described in #1321 ("some open loop is keeping the process alive").

It also isn't specific to forks or to these PRs — #1527 and #1530, both branched inside this repo, fail the same job, while #1528, #1529, #1532 and #1533 pass it. Looks intermittent.

Unit tests and type-checks for the packages touched here are green locally, and identical to a clean checkout.

@CaptainAni187

Copy link
Copy Markdown
Author

The red integration_test-22.20 looks like flake rather than this branch. Same job passes on 26.5.1 and 24.18.1 here, and across other open PRs it fails on a different node version each time: #1547 fails 24.18.1 but passes 22.20, #1519 fails two of three, #1521 and #1534 pass all three.

This change is a pure string function in packages/project/src/util/slugify.ts, so a failure on one node version while the other two pass would be hard to explain. Happy to push an empty commit to re-run it if that helps.

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.

CLI slugify drops non-ASCII characters from job and workflow identifiers

1 participant