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

fix(builder): Module.addLayout/nuxt.config take precedence over auto-scanned layouts #4702

Merged
merged 5 commits into from Jan 9, 2019

Conversation

Projects
None yet
6 participants
@clarkdo
Copy link
Member

clarkdo commented Jan 7, 2019

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Fix issue reported by #4641 and #4647 (comment)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@clarkdo clarkdo changed the title fix(builder): Module.addLayout/nuxt.config take precedence over auto-… fix(builder): Module.addLayout/nuxt.config take precedence over auto-scanned layouts Jan 7, 2019

@clarkdo clarkdo requested review from manniL , pi0 and Atinux and removed request for manniL Jan 7, 2019

@clarkdo clarkdo referenced this pull request Jan 7, 2019

Open

feat: .nuxtignore #4647

7 of 9 tasks complete
@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

Nice, we should be able to overwrite a layout without the need for a warning, do not you think?
From what I've looked at, this condition is not necessary || /\.vue$/.test(file), since js, vue files are already filtered and this would solve all problems

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 7, 2019

Codecov Report

Merging #4702 into dev will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #4702      +/-   ##
==========================================
+ Coverage   90.45%   90.48%   +0.02%     
==========================================
  Files          67       67              
  Lines        2252     2259       +7     
  Branches      555      557       +2     
==========================================
+ Hits         2037     2044       +7     
  Misses        194      194              
  Partials       21       21
Impacted Files Coverage Δ
packages/builder/src/builder.js 93.62% <100%> (+0.09%) ⬆️
packages/core/src/module.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c018a...65cc981. Read the comment docs.

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Jan 7, 2019

Nice, we should be able to overwrite a layout without the need for a warning, do not you think?

In this case, layout has been defined in addLayout, I suppose it has higher priority than auto-scanned layout, how do you think ?

From what I've looked at, this condition is not necessary || /\.vue$/.test(file), since js, vue files are already filtered and this would solve all problems

The condition is for making layouts/demo.vue takes precedence over layouts/demo.js.

@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jan 7, 2019

@ricardogobbosouza I think a warning is fine there. Are there use-cases where you want to override the layout and not delete the overridden file? 🤔

@Atinux

Atinux approved these changes Jan 7, 2019

Copy link
Member

Atinux left a comment

Nice job

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

@clarkdo I think we can remove the builder warning and just put in addLayout and add a third parameter override = false and change condition if (layout && !override) {

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

@manniL Yes, I believe that the module should be able to replace a layout without the need for a warning

@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jan 7, 2019

@ricardogobbosouza I got that. But can you explain me in which scenario(s) that'd be helpful when the user simply could delete or rename the file being overriden?

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

@manniL I would like the module to replace a layout without the need to post a warning.
In my case, I have a module that changes the default layout

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Jan 7, 2019

@ricardogobbosouza The reason I add the warning is for reminding user that he should remove the layouts/layout or disable module, it may make user have a better overall understanding of his project than hide the details, how do you think ?

@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jan 7, 2019

@ricardogobbosouza Well, overriding the default layout (if it hasn't been defined before) probably shouldn't trigger a warning.

But other than that I don't see an advantage in not triggering a warning as the user does not necessarily know that it's layouts are overridden otherwise.

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

I think we could have a third parameter to disable the warning when necessary, by default it is activated

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Jan 7, 2019

@ricardogobbosouza If we add the parameter, the control is still in lib author side instead of user side and the underlying override behaviour is still invisible to users 😄

@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jan 7, 2019

@ricardogobbosouza I wanted to suggest that first too but most of the time the module creator is not the actual "module consumer" (and the latter should govern the enabled warnings).

Still, I want to understand in which situations a disabled warning would make sense (from your point of view). Could you please explain?

Feel free to join the Discord for a faster communication ☺️

clarkdo added some commits Jan 7, 2019

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 7, 2019

@clarkdo, you're right!
What if I add a setting in the options? This would be in control of the user in nuxt.config.js

@manniL In my case I have a module that always replaces a layout and in all my projects I'm going to receive this notice.

But if you do not find it necessary, this already solves my problem by adding an existing layout

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Jan 7, 2019

@ricardogobbosouza In your special case, I think you can replace the layout file in layouts dir instead of using addLayout 😃

But I totally understand your concern, it makes sense in lib-author side.

Let's ask for @Atinux and @pi0 's thought as well.

@manniL

manniL approved these changes Jan 7, 2019

@Atinux

This comment has been minimized.

Copy link
Member

Atinux commented Jan 8, 2019

Actually, we should not forget about module authors since it is what makes Nuxt so cool to use.

Actually, the warning popup only when we use addLayout (for example a module) AND if a file with the same name is used right @clarkdo ?

I do believe that is a module add a layout, it should be prefixed anyway to avoid conflicts, or, exposes a wrapper component that can be used in user layouts.

WDYT?

@clarkdo

This comment has been minimized.

Copy link
Member

clarkdo commented Jan 8, 2019

Actually, the warning popup only when we use addLayout (for example a module) AND if a file with the same name is used right?

It's right, but actually user can define layouts by nuxt.config which isn't in our doc:

layouts: {
  custom: path.resolve(__dirname, './customDir/custom')
}

I do believe that is a module add a layout, it should be prefixed anyway to avoid conflicts, or, exposes a wrapper component that can be used in user layouts.

Yes, its correct, but we cannot add the prefix automatically, because user won't find the correct layout name, e.g.:

module.addLayout('./templates/custom')

// Then we add prefix, so the name may be like: `module-custom`

// In page
export default {
  layout: 'custom' // this won't work
  layout: 'module-custom' // this is correct
}

So maybe the prefix should be added in module-author level ?
Or we need to make a convention for the module layout name or return name in addLayout.

@manniL

This comment has been minimized.

Copy link
Member

manniL commented Jan 8, 2019

Then the question is: "What is the module name"? How can we apply that consistently and still "easy to discover"? 🤷‍♂️

I think that'd delegate the problem to a new problem.

@ricardogobbosouza

This comment has been minimized.

Copy link
Contributor

ricardogobbosouza commented Jan 8, 2019

In my opinion the name of the layout should not be prefixed automatically, this would not allow a module overwriting a layout at the root of the project.

@pi0

This comment has been minimized.

Copy link
Member

pi0 commented Jan 9, 2019

Merging this to prevent stale. Feel free opening an Issue/PR/RFC for behavior improvements. PS with module improvements (nuxt/rfcs#10) module container is being deprecated.

@pi0 pi0 merged commit f85ac94 into nuxt:dev Jan 9, 2019

10 checks passed

Semantic Pull Request ready to be squashed
Details
[ci.azure] nuxt.js #20190108.4 succeeded
Details
ci/circleci: audit Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint-app Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk - package.json (Atinux) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment