noaignite / create-ignite-app

Boilerplate for React with Next.js and MUI
17 stars 2 forks source link

move `~/containers/Page` as subsidiary to `~/layouts` #90

Closed adamsoderstrom closed 1 year ago

adamsoderstrom commented 1 year ago

I interpret the Page entity to not be equal to the other containers, therefore i changed it to be subsidiary to the layout directory. This is because the Page entity is almost always expected to be rendered inside a layout. This solves the dependency cycle.

An alternative approach would be keeping the Page entity as a container, but not exporting it in ~/containers/index.js

maeertin commented 1 year ago

Nice find @adamsoderstrom ✨ I'll look closer into this on Monday.

alexanderflink commented 1 year ago

I think we should just remove the Page export from containers until we find a solution to this "root export" dependency cycle problem. Martin mentioned we could have an eslint rule that tells you to not do absolute imports from containers within the containers folder, but rather use relative imports.

adamsoderstrom commented 1 year ago

Ah, right, @alexanderflink!

I've create another PR, which removes the export from ~/containers (#91).

maeertin commented 1 year ago

I do think Page is more of a container than a layout based on current definitions, so I would suggest like Alex says, leaving it as a container but removing the Page export from src/containers/index.js.

adamsoderstrom commented 1 year ago

Right. Will close this, hence we won't want to move the Page entity to the layouts directory.

maeertin commented 1 year ago

Thank you ✨