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

refactor: use region-helper #784

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor: use region-helper #784

wants to merge 4 commits into from

Conversation

eunjae-lee
Copy link

@eunjae-lee eunjae-lee commented Feb 14, 2024

Pull request type

Jira Link: EXT-2185

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Other (please describe):

How to test this PR

Nothing changes. It should pass all the existing tests.

What is the new behavior?

This PR includes the new @storyblok/region-helper library to replace the existing region-related code in this repository. The purpose is to centralize the logic into a single library, and reuse it everywhere (to ease the situation where we add new regions).

You can refer to the internal #product-regions-configuration channel or reach out to me for more information.

Basically the library's code is copied from this repository :)

Also, I've ran npm run build locally, and confirmed that the region-helper was correctly bundled into the final output.

@eunjae-lee
Copy link
Author

I think the CI is not working ... because I'm an external contributor? Let me know if I should do something else :)

Copy link
Member

@ademarCardoso ademarCardoso left a comment

Choose a reason for hiding this comment

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

The code it's working great, i will ask to my QA review for us and then we can merge this, Thanks @eunjae-lee .

@eunjae-lee
Copy link
Author

just added a commit to upgrade region-helper to 0.2.0, but nothing really changes :)

@BibiSebi
Copy link

Hey @ademarCardoso, can I support you here in any way (like finding or assigning a QA)? :) @eunjae-lee will be out of the office for the rest of the week so I will be glad to help if anything is needed.

src/index.ts Show resolved Hide resolved
@BibiSebi BibiSebi self-requested a review February 27, 2024 14:37
@BibiSebi
Copy link

@eunjae-lee @ademarCardoso I added some changes to make sure that it is still possible to switch between http and https.

ademarCardoso
ademarCardoso previously approved these changes Feb 28, 2024
Copy link
Member

@ademarCardoso ademarCardoso left a comment

Choose a reason for hiding this comment

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

Hi @BibiSebi I've reviewed it again and also looked at the code once more in the region-helper repo, and everything looks perfect to me 🚀

package.json Show resolved Hide resolved
@ademarCardoso
Copy link
Member

Hi @eunjae-lee and @BibiSebi , after checking with the team, we won't be releasing this change. As I said in my reviews, the change is interesting and makes sense, but as in storyblok-js-client we try to keep the final bundle as small as possible, we think that here the best case is not to add this code, because js-client does not have any external dependencies and we want to keep it that way so that we continue with the performance and the smallest size of the final bundle.
Thanks for your contribution ❤️ , if you need anything just give me a call. 😄

@eunjae-lee
Copy link
Author

Hi @ademarCardoso and @ts-storyblok ,
I was away last week attending a conference, and now I'm back.

I totally understand your concern. It's a good strategy not including third party dependencies that could increase the bundlesize and have potential security risk, or uncontrollable situations, etc.

However, this region-helper is a tiny library maintained by ourselves, and totally treeshakable. And I have confirmed that it actually hasn't increase the bundle size in this pull request.

For example, when I created this pull request, I created this branch based on the commit 3f9fb78, and if you

(A)

git checkout 3f9fb78
npm install
npm build

(B)

git checkout refactor/EXT-2185
npm install && npm build

We can see introducing this library didn't have any impact on the bundle size (the difference between (A) and (B) here)

File Size - (A) Size - (B) Gzip Size - (A) Gzip Size - (B)
dist/index.mjs 26.11 25.93 7.69 7.63
dist/index.umd.js 18.46 18.47 6.53 6.53

Bundlesize aside, it gets harder to maintain all the newly added regions (in the future) unless we centralize our effort. Let me know what you think! 🙂

@eunjae-lee eunjae-lee dismissed ademarCardoso’s stale review March 5, 2024 13:42

would love to get your another feedback on this. thanks :)

Copy link
Member

@ademarCardoso ademarCardoso left a comment

Choose a reason for hiding this comment

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

My concern about dependency still remains, but I think we can go down that road and test it, thanks for the feedback @eunjae-lee . Good work on the region helper and here 🚀

@eunjae-lee
Copy link
Author

Thanks @ademarCardoso !

Then what's the next step? Will a QA be involved here? Or are we merging this? Let me know if I need to do anything :)

@BibiSebi
Copy link

@eunjae-lee after looking a bit at the github actions, i believe they will handle the release once this PR is merged to main.

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