Libs tidy up part 5 - Move core to lib #1734

arelra · 2025-01-09T18:47:25Z

What does this change?

Following #1729 we now have two main locations for lib type modules:

This PR moves all core modules to lib and also creates a top level export.ts which exports all the functions required by clients of the commercial library.

Before	After
`- core` `- lib` `- lib`	`- lib` `export.ts`

The dist output has also been updated:

Before	After
`- dist` `- esm` `- core`	`- dist` `- core` `- esm`

Why?

Consolidate all libs and utils to one part of the codebase

changeset-bot · 2025-01-09T18:47:29Z

⚠️ No Changeset found

Latest commit: 7a7ffe1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-01-09T18:49:20Z

Deploy build 2922 of `dotcom::commercial-bundle` to CODE

All deployment options

From guardian/actions-riff-raff.

github-actions · 2025-01-09T18:50:31Z

Ad load time test results

For consented, top-above-nav took on average 4470ms to load.
For consentless, top-above-nav took on average 3101ms to load.

Test conditions:

5mbps download speed
1.5mbps upload speed
150ms latency

Jakeii · 2025-01-10T14:42:30Z

We still have the 2 parts, bundle and the core/libs used by other projects, do we not want to keep them seperate in some way?
I was thinking we might even split them into workspaces in here at some point.

github-actions · 2025-01-14T17:22:58Z

🚀 0.0.0-beta-20250114172246 published to npm as a beta release

To install the beta version, run one of the following commands in your project directory:

npm install @guardian/[email protected]
pnpm add @guardian/[email protected]
yarn add @guardian/[email protected]

arelra · 2025-01-14T17:29:34Z

We still have the 2 parts, bundle and the core/libs used by other projects, do we not want to keep them seperate in some way? I was thinking we might even split them into workspaces in here at some point.

I don't see the need for the distinction any more - its just libs? We have some lib functions which we use internally and also export e.g. build-page-targeting. I think it made sense when core was the newer, stricter TS set of libs but now the whole codebase follows the same TS rules. The only difference is what we choose to export which is already made explicit by export.ts.

Is there any advantage to splitting 'core' and the rest of the codebase?

Jakeii · 2025-01-15T09:50:43Z

We still have the 2 parts, bundle and the core/libs used by other projects, do we not want to keep them seperate in some way? I was thinking we might even split them into workspaces in here at some point.

I don't see the need for the distinction any more - its just libs? We have some lib functions which we use internally and also export e.g. build-page-targeting. I think it made sense when core was the newer, stricter TS set of libs but now the whole codebase follows the same TS rules. The only difference is what we choose to export which is already made explicit by export.ts.

Is there any advantage to splitting 'core' and the rest of the codebase?

If merged, it may be hard to distinguish if a change will affect an exported module and thus require a changeset and version bump.

I also just like the mental distinction, they feel live quite separate things to me, maybe we can see what others think?

arelra · 2025-01-15T12:32:07Z

If merged, it may be hard to distinguish if a change will affect an exported module and thus require a changeset and version bump.

I also just like the mental distinction, they feel live quite separate things to me, maybe we can see what others think?

Thanks

core vs lib

I can see it helps to separate the idea of 'core' but I think my main point is that its hard to define the line between core and the rest of the codebase.

I suspect the confusion would remain as core modules also depend on the main 'lib' so its hard to know if any updates affect the call chain that ends up in 'core' changing. So to have a clear idea of what can affect core would mean pulling the complete dependency chain into core.

Also its a side point but core is quite a mixed bag, there are modules in there that are not exported - e.g. messenger, third-party-tags. There are duplicate libs e.g. geolocation and url parsing which is hard to see without consolidation.

changesets

I've tended to use changesets to indicate breaking, minor, patch of the bundle and core, rather than just being specific to core, for example when upgrading prebid.

The riff-raff deploy changes the visibility of the version but its still useful to understand the implication of a change to bundle.

Would be good to get other opinions!

cemms1

LGTM - does the renaming and reorganising as described in the PR description. Though I'm not very familiar with the commercial code yet so can't review from a more specific angle!

EDIT: I've just read the other comments on this PR, sorry for jumping the gun with my review before. I think since I haven't used commercial in practice I'm not the best person to approve this!

cemms1 · 2025-01-15T12:55:11Z

package.json

 	"scripts": {
 		"build": "npm-run-all clean --parallel compile:core:* build:prod build:dev build:riff-raff",
 		"build:dev": "webpack -c webpack.config.dev.mjs",
 		"build:prod": "webpack -c webpack.config.prod.mjs",
 		"build:riff-raff": "webpack -c webpack.config.riff-raff.mjs",
 		"clean": "rm -rf dist",
-		"compile:core:common": "tsc --project ./tsconfig.core.json --outDir ./dist/cjs --module CommonJS",
-		"compile:core:esm": "tsc --project ./tsconfig.core.json --outDir ./dist/esm",
+		"compile:core:common": "tsc --project ./tsconfig.core.json --outDir ./dist/core/cjs --module CommonJS",


Is common still used? Since the filepath for main on L17 has been updated from cjs to esm

Oops! Thanks good spot

I don't think the cjs build is used by any clients of commercial (Frontend or DCR). Removing it is on my todo list but will do separately to keep this PR focussed.

Opening up the floor to other commercial developers

emma-imber · 2025-01-15T13:06:40Z

Just weighing in on the separation of core vs lib. I'm personally in favour of merging them. I regularly get a bit frustrated when working with things like page targeting, and having to look at files in core and in lib. The separation can make it harder for me to find the file I need.

I do understand the point around making it clear what changes would result in a bump to 'core' in DCR. However, I guess we can't say for sure that commercial will still be used in exactly the same way as it is now a year or two down the line. I agree that it makes sense for us to keep using changesets for all changes, even if they're not in 'core'. I think that futureproofs us a little better as well as keeps our changes nice and transparent!

Jakeii · 2025-01-15T13:11:14Z

Okay I'm convinced! Thanks for weighing in everyone :)

github-actions · 2025-01-15T14:12:12Z

🚀 0.0.0-beta-20250115141200 published to npm as a beta release

To install the beta version, run one of the following commands in your project directory:

npm install @guardian/[email protected]
pnpm add @guardian/[email protected]
yarn add @guardian/[email protected]

deedeeh

I checked the PR comments and I definitely need to understand a bit more about how core and lib are being used and then I will be able to give my opinion on it. I will make sure to put some time tomorrow to clear my understanding. If I have any questions will post it on the chat.

arelra added 5 commits January 9, 2025 17:47

Move third-party-tags from core to lib

85e56d3

Move remaining core modules to lib

d1fd543

Create export.ts to export libs

633f8c3

Move get-locale to lib/geo - duplicate of country-code

49ebab7

Move all previous core/lib modules now in lib/lib up to lib

7bd4019

Update dist core path and entry point to export.js

d281b9b

arelra added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jan 14, 2025

github-actions bot removed the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jan 14, 2025

arelra marked this pull request as ready for review January 15, 2025 06:14

arelra requested a review from a team as a code owner January 15, 2025 06:14

arelra mentioned this pull request Jan 15, 2025

Doc explaining the codebase structure #1743

Open

cemms1 previously approved these changes Jan 15, 2025

View reviewed changes

cemms1 requested review from a team and cemms1 and removed request for cemms1 January 15, 2025 12:59

cemms1 removed their request for review January 15, 2025 13:01

Fix cjs dist entry point

7a7ffe1

arelra added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jan 15, 2025

github-actions bot removed the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jan 15, 2025

deedeeh reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Libs tidy up part 5 - Move core to lib #1734

Libs tidy up part 5 - Move core to lib #1734

arelra commented Jan 9, 2025 •

edited

Loading

changeset-bot bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

Jakeii commented Jan 10, 2025

github-actions bot commented Jan 14, 2025

arelra commented Jan 14, 2025

Jakeii commented Jan 15, 2025

arelra commented Jan 15, 2025 •

edited

Loading

cemms1 left a comment •

edited

Loading

cemms1 Jan 15, 2025

arelra Jan 15, 2025

emma-imber commented Jan 15, 2025

Jakeii commented Jan 15, 2025

github-actions bot commented Jan 15, 2025

deedeeh left a comment

Libs tidy up part 5 - Move core to lib #1734

Are you sure you want to change the base?

Libs tidy up part 5 - Move core to lib #1734

Conversation

arelra commented Jan 9, 2025 • edited Loading

What does this change?

Why?

changeset-bot bot commented Jan 9, 2025 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jan 9, 2025 • edited Loading

Deploy build 2922 of dotcom::commercial-bundle to CODE

github-actions bot commented Jan 9, 2025 • edited Loading

Ad load time test results

Jakeii commented Jan 10, 2025

github-actions bot commented Jan 14, 2025

arelra commented Jan 14, 2025

Jakeii commented Jan 15, 2025

arelra commented Jan 15, 2025 • edited Loading

core vs lib

changesets

cemms1 left a comment • edited Loading

Choose a reason for hiding this comment

cemms1 Jan 15, 2025

Choose a reason for hiding this comment

arelra Jan 15, 2025

Choose a reason for hiding this comment

emma-imber commented Jan 15, 2025

Jakeii commented Jan 15, 2025

github-actions bot commented Jan 15, 2025

deedeeh left a comment

Choose a reason for hiding this comment

arelra commented Jan 9, 2025 •

edited

Loading

changeset-bot bot commented Jan 9, 2025 •

edited

Loading

github-actions bot commented Jan 9, 2025 •

edited

Loading

Deploy build 2922 of `dotcom::commercial-bundle` to CODE

github-actions bot commented Jan 9, 2025 •

edited

Loading

arelra commented Jan 15, 2025 •

edited

Loading

cemms1 left a comment •

edited

Loading