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

WSTEAM1-1453 Update imports to resolve all import/no-relative-packages errors #12224

Merged
merged 72 commits into from
Dec 11, 2024

Conversation

Louis-Matsika
Copy link
Contributor

@Louis-Matsika Louis-Matsika commented Dec 4, 2024

Resolves JIRA [1453]

Overall changes

Update imports to fix all occurences of Relative import from another package is not allowed error to help unblock Upgrade eslint-config-airbnb to 19.0.4

list of files that need to be updated as listed in Jira: paper doc

Code changes

for each file listed:
updated import statments to use nextjs module aliases # Instead of relative pathing ../../../

our current aliases are:

      ['#app', './src/app'],
      ['#contexts', './src/app/contexts'],
      ['#components', './src/app/legacy/components'],
      ['#containers', './src/app/legacy/containers'],
      ['#data', './data'],
      ['#hooks', './src/app/hooks'],
      ['#lib', './src/app/lib'],
      ['#psammead', './src/app/legacy/psammead'],
      ['#models', './src/app/models'],
      ['#pages', './src/app/pages'],
      ['#testHelpers', './src/testHelpers'],
      ['#server', './src/server'],
      ['#storybook', './.storybook'],

A new Alias: ['#src', './src'] has been created that should allow access to every file in /src. allowing for some paths that previously couldn’t be updated to now be amended e.g /src/testHelpers

A new Alias: ['#next', './ws-nextjs-app'] has been created that should allow access to every file in /ws-nextjs-app. allowing for some paths that previously couldn’t be updated to now be amended e.g /ws-nextjs-app/pages/[service]/live/[id]/Post/types

Testing

see github checks on this commit, expand the linting checks & search for Relative import from another package is not allowed. no results should come up as these errors should be fixed now

Helpful Links

Add Links to useful resources related to this PR if applicable.

Nextjs module aliases docs

Coding Standards

Repository use guidelines

@Louis-Matsika Louis-Matsika self-assigned this Dec 6, 2024
@Louis-Matsika Louis-Matsika changed the title 1453 Update imports to resolve all import/no-relative-packages errors WSTEAM1-1453 Update imports to resolve all import/no-relative-packages errors Dec 9, 2024
@Louis-Matsika Louis-Matsika added dependencies technical-work Technical debt, support work and building new technical tools and features labels Dec 9, 2024
Comment on lines 23 to 25
import gahuzaOnDemandAudio from '#data/gahuza/bbc_gahuza_radio/p02pcb5c.json';
// eslint-disable-next-line import/order
import routes from '.';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

import routes from '.'; seems to need to be above the alias imports from below, however the linter doesn't like this.

To get both the linter passing and path aliasing to work I had to use this command // eslint-disable-next-line import/order

I looked if there are other instances of this exact command in Simorgh, there isn't. but there are many instances of commands similar that start with eslint-disable-next-line…

There is however an instance of a command to disable the import order for a whole file /* eslint-disable import/order */ in src/server/utilities/customMetrics/index.test.ts

The imports from #app and #psammead in this file specifically didn’t get flagged for needing an alias in Karina's PR #12206 that shows where errors for Relative import from another package is not allowed. are. if we don’t want to use this // eslint-disable-next-line import/order workaround we i don’t think we have to. Just thought I’d change them to use aliases anyway.

We could leave these imports as they were previously and I think they still might not show up when linting, as they weren't flagged before?

Don’t know how people feel about this?

karinathomasbbc and others added 3 commits December 11, 2024 13:32
…-config-airbnb to 19.0.4 to ensure that all `import/no-relative-packages` errors have been resolved.
…hub.com:bbc/simorgh into 1453-update-imports
@karinathomasbbc
Copy link
Contributor

I have confirmed that upgrading eslint-config-airbnb to 19.0.4 no longer contains any import/no-relative-packages - see https://github.com/bbc/simorgh/actions/runs/12277526136/job/34257105857?pr=12224, expand the linting & search for import/no-relative-packages

Screenshot 2024-12-11 at 13 51 47

I will now revert 374e0da

…e eslint-config-airbnb to 19.0.4 to ensure that all `import/no-relative-packages` errors have been resolved."

This reverts commit 374e0da.
@Louis-Matsika Louis-Matsika merged commit f02d40d into latest Dec 11, 2024
11 checks passed
@Louis-Matsika Louis-Matsika deleted the 1453-update-imports branch December 11, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies technical-work Technical debt, support work and building new technical tools and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants