Add option to fall back on regex parsing to splitAddress1 #281

gabypancu · 2024-08-30T13:57:59Z

What are you trying to accomplish?

Resolves https://github.com/Shopify/address/issues/2726

When there is no reserved delimiter present in an address line (i.e. any address that was created before the release of additional address fields), we want to attempt to split the additional fields out using regex. The regex patterns will have to be strict in order to minimize incorrect captures.

What approach did you choose and why?

Adds a conservative address1 regex pattern for the Netherlands. On a sample of about 800 address lines, it parses 75% correctly (the rest are not matched) - https://rubular.com/r/3BNDUviIL8hHeS
~~2. Adds a splitAddress1WithFallback function to the TS library that uses the regex as a fallback if splitting on the delimiter is not possible~~
Adds an optional param tryRegexFallback to the existing splitAddress1 function in the TS library. When true, and when the input string does not contain the reserved delimiter, we attempt to parse using regex.

This way, checkout web still only has one function to call for splitting.

The following PRs have been merged into this one:

See PR comments for debate on the following pros and cons
Our options for where to implement this new behaviour:

In the existing splitAddress1 function, controlled by a new optional param

Requires the smallest change in checkout-web, but brings higher visibility to this feature
Reusability if we choose to use this on surfaces other than checkout-web

In a new function in Worldwide

Maintains separation with the existing splitting function, reduces visibility to this feature
Reusability if we choose to use this on surfaces other than checkout-web

Directly in checkout-web when splitting fails

Keeps this feature private to Shopify
Introduces complexity & Worldwide domain into checkout-web
Not reusable outside of checkout-web

Checklist

I have added a CHANGELOG entry for this change (or determined that it isn't needed)

db/data/regions/NL.yml

lang/typescript/src/extended-address/splitAddress1WithFallback.test.ts

lang/typescript/src/utils/address-fields.ts

lang/typescript/src/extended-address/splitAddress1WithFallback.ts

lang/typescript/src/utils/address-fields.ts

lang/typescript/src/extended-address/splitAddress1.ts

db/data/regions/NL.yml

lib/worldwide/region.rb

lang/typescript/src/extended-address/splitAddress1.test.ts

DominiqueFlaaa · 2024-09-06T15:25:04Z

lang/typescript/CHANGELOG.md

+## 0.7.0-next.0
+
+### Minor Changes
+
+- 1f5d405: Add optional experimentalRegexFallback param to splitAddress1 function to attempt splitting address lines that do not contain the reserved delimiter
+


Did you follow these docs to generate the prerelease? https://github.com/changesets/changesets/blob/main/docs/prereleases.md

Looks like it was but I don't see the package published as a prerelease on NPM so might have to change something in the GH Actions to actually do it https://www.npmjs.com/package/@shopify/worldwide?activeTab=versions

Ah I skipped the publish step for now - wasn't sure if that should happen after this PR merges

I'll have to read the prerelease again but I though they recommended not merging something in prerelease mode to the main/default branch and rather just have it deploy from a separate branch, then exit prerelease mode before merging.

Ok yeah changesets does recommend using a second branch when doing prereleases just so the main branch isn't locked up in prerelease mode. Given we don't make that many changes to this library and we won't be in prerelease for long thats probably still fine to just lock up the main branch but I'd rather not if we don't have to so main stays in a releasable state that reflects the latest version.

We can try to run the publish on this branch, we'll need to modify the GH action to also run on this branch though so the job that handles publishing will run.

lang/typescript/src/extended-address/splitAddress1.ts

lang/typescript/src/extended-address/splitAddress1.test.ts

lang/typescript/src/utils/address-fields.ts

kennygoff

LGTM 👍 If we want to do the prerelease I still recommend trying to publish the prerelease from this branch instead of merging though as the changesets docs recommend.

DominiqueFlaaa · 2024-09-11T13:33:14Z

lang/typescript/.changeset/pre.json

+ "mode": "pre",
+ "tag": "next",
+ "initialVersions": {
+ "@shopify/worldwide": "0.6.0"


Should this be "0.7.0"?

No, this is the initial version before the pre-release. I looked at the tests in changesets, you can see they've set up the initial package versions here, and this is the version that appears in the release plan's initialVesrions https://github.com/changesets/changesets/blob/c867f32d0c25cfb2a6b518bc1f9880f758beb157/packages/assemble-release-plan/src/index.test.ts#L908

DominiqueFlaaa · 2024-09-11T13:37:39Z

test/worldwide/region_yml_consistency_test.rb

@@ -192,6 +192,21 @@ class RegionYmlConsistencyTest < ActiveSupport::TestCase
 end
 end

+ test "address1_regex capture groups must belong to a limited set of allowed names" do


👨‍🍳 👌

DominiqueFlaaa · 2024-09-11T13:42:10Z

lang/typescript/src/utils/address-fields.test.ts

+ test('creates an address object from string matching one of the defined regexes', () => {
+ const fieldDefinition: FieldConcatenationRule[] = [
+ {key: 'streetName'},
+ {key: 'streetNumber'},
+ ];
+ const regexPatterns = [
+ new RegExp('^(?<streetName>[^\\d]+) (?<streetNumber>\\d+)$'),
+ ];


nit: can you update this test to have 2 regexPatterns, with the first not a match and the second a match?

DominiqueFlaaa

Excellent work, Gaby! Left a couple comments, non-blocking.

gabypancu · 2024-09-12T15:11:03Z

The pre-release is now published as v0.7.0-next.0.

Next steps:

Keep this PR open, merge any additional regex support into this branch
Start using the pre-release version in checkout-web
Once this is tested on checkout and we have support for all relevant countries, exit pre-release mode, publish the new version and merge this PR into main
a. Don't forget to remove the branch from npm-release.yml

.github/workflows/npm-release.yml

DominiqueFlaaa · 2024-10-01T15:06:07Z

lang/typescript/CHANGELOG.md

+- bbacde1: Add address1 regex for BE, BR, CL, ES, IL, MX
+- b6e1c7f: Add address1 regex for DE
+- ee7b7a9: Add optional tryRegexFallback param to splitAddress1 function to attempt splitting address lines that do not contain the reserved delimiter
+
+## 0.7.0-next.2
+
+### Minor Changes
+
+- 3bfb56a: Add address1 regex for DE
+
+## 0.7.0-next.1
+
+### Minor Changes
+
+- Add address1 regex for BE, BR, CL, ES, IL, MX
+
+## 0.7.0-next.0
+
+### Minor Changes
+
+- 1f5d405: Add optional tryRegexFallback param to splitAddress1 function to attempt splitting address lines that do not contain the reserved delimiter


Should we remove the pre-releases, since we are no longer publishing a pre-release in favour of a minor version?

The pre-releases are published and available in npm, so although 0.7.0 includes the same content I think it's worth including this bit of documentation on the pre-release versions. I see it left in in some other shopify repos

https://github.com/Shopify/libsql/blob/54ff4218cc4178021169179f3c7f528da6bfb1ae/libsql-sqlite3/ext/crr/CHANGELOG.md#L32

https://github.com/Shopify/app-bridge/blob/6abdc86736708395bc252221da462016c4f51fa1/examples/app-bridge-playground/CHANGELOG.md#L1057

The pre-releases are published and available in npm

Ah thank you for clarifying.

…ckage

DominiqueFlaaa

LGTM!

gabypancu force-pushed the parse-additional-fields branch from 4a91ae4 to 7cd3b5f Compare August 30, 2024 19:40

gabypancu changed the title ~~Parse additional fields~~ Add parseAddress1 function to parse standard addresses Aug 30, 2024

gabypancu force-pushed the parse-additional-fields branch from 67847ed to 4162f5f Compare September 4, 2024 15:46

gabypancu marked this pull request as ready for review September 4, 2024 16:04

gabypancu changed the title ~~Add parseAddress1 function to parse standard addresses~~ Add splitAddress1WithFallback function to parse standard addresses Sep 4, 2024

gabypancu changed the title ~~Add splitAddress1WithFallback function to parse standard addresses~~ Add splitAddress1WithFallback function Sep 4, 2024

gabypancu commented Sep 4, 2024

View reviewed changes

db/data/regions/NL.yml Outdated Show resolved Hide resolved

gabypancu requested review from rochlefebvre, kennygoff and DominiqueFlaaa September 4, 2024 16:15

kennygoff requested changes Sep 4, 2024

View reviewed changes

gabypancu commented Sep 5, 2024

View reviewed changes

lang/typescript/src/extended-address/splitAddress1.ts Outdated Show resolved Hide resolved

gabypancu requested a review from kennygoff September 5, 2024 19:23

gabypancu changed the title ~~Add splitAddress1WithFallback function~~ Add option to fall back on regex parsing to splitAddress1 Sep 6, 2024

DominiqueFlaaa reviewed Sep 6, 2024

View reviewed changes

kennygoff requested changes Sep 6, 2024

View reviewed changes

gabypancu requested review from kennygoff and DominiqueFlaaa September 10, 2024 20:43

kennygoff approved these changes Sep 11, 2024

View reviewed changes

DominiqueFlaaa reviewed Sep 11, 2024

View reviewed changes

DominiqueFlaaa approved these changes Sep 11, 2024

View reviewed changes

gabypancu force-pushed the parse-additional-fields branch 2 times, most recently from 76d8165 to 7829478 Compare September 12, 2024 15:02

gabypancu mentioned this pull request Sep 18, 2024

Add address1 Regex for remaining countries #282

Merged

1 task

gabypancu force-pushed the parse-additional-fields branch from 5222d46 to ed6edce Compare October 1, 2024 12:12

gabypancu commented Oct 1, 2024

View reviewed changes

.github/workflows/npm-release.yml Outdated Show resolved Hide resolved

gabypancu requested a review from DominiqueFlaaa October 1, 2024 13:45

DominiqueFlaaa reviewed Oct 1, 2024

View reviewed changes

gabypancu force-pushed the parse-additional-fields branch from edc19bf to ba7c69f Compare October 1, 2024 16:40

add regex fallback to splitAddress1 function, release v0.7.0 of TS pa…

b95457f

…ckage

gabypancu force-pushed the parse-additional-fields branch from f433fc9 to b95457f Compare October 1, 2024 16:45

DominiqueFlaaa approved these changes Oct 1, 2024

View reviewed changes

gabypancu merged commit f13cd4c into main Oct 1, 2024
10 checks passed

gabypancu deleted the parse-additional-fields branch October 1, 2024 16:53

shopify-shipit bot temporarily deployed to rubygems October 1, 2024 17:29 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to fall back on regex parsing to splitAddress1 #281

Add option to fall back on regex parsing to splitAddress1 #281

gabypancu commented Aug 30, 2024 •

edited

Loading

DominiqueFlaaa Sep 6, 2024

kennygoff Sep 6, 2024

gabypancu Sep 6, 2024

kennygoff Sep 6, 2024

kennygoff Sep 6, 2024

kennygoff left a comment

DominiqueFlaaa Sep 11, 2024

gabypancu Sep 12, 2024 •

edited

Loading

DominiqueFlaaa Sep 11, 2024

DominiqueFlaaa Sep 11, 2024

DominiqueFlaaa left a comment

gabypancu commented Sep 12, 2024

DominiqueFlaaa Oct 1, 2024

gabypancu Oct 1, 2024

DominiqueFlaaa Oct 1, 2024

DominiqueFlaaa left a comment

Add option to fall back on regex parsing to splitAddress1 #281

Add option to fall back on regex parsing to splitAddress1 #281

Conversation

gabypancu commented Aug 30, 2024 • edited Loading

What are you trying to accomplish?

What approach did you choose and why?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennygoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabypancu Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominiqueFlaaa left a comment

Choose a reason for hiding this comment

gabypancu commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominiqueFlaaa left a comment

Choose a reason for hiding this comment

gabypancu commented Aug 30, 2024 •

edited

Loading

gabypancu Sep 12, 2024 •

edited

Loading