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

fix(module): warn about head as a function in nuxt config is not supported #14

Merged
merged 4 commits into from Feb 26, 2020

Conversation

hagi4u
Copy link
Contributor

@hagi4u hagi4u commented Feb 26, 2020

This PR is solved problem to execute when head property is function in nuxt.config.js and then added head property code in nuxt.config.js which necessary test codes

@hagi4u
Copy link
Contributor Author

hagi4u commented Feb 26, 2020

image

test results.

lib/module.js Outdated
@@ -51,6 +51,7 @@ module.exports = async function gtmModule (_options) {
script = `if(!window._gtm_init){window._gtm_init=1;${script}}`

// Add google tag manager <script> to head
this.options.head = typeof this.options.head === 'function' ? {} : this.options.head
Copy link
Member

Choose a reason for hiding this comment

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

This silently removes user-provided head function. Meanwhile working on a nuxt enhancement to allow providing a custom head in a better way (app/head.js) maybe adding a warning like [@nuxtjs/gtm] head is provided as a function which is not supported by this module at the moment. Removing user-provided head. (or something like this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i found it. so i try to fix this right way that support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think that your better way is good alternative. so i'm gonna remove to my head function in nuxt.config.js. Inadvertently i made you confused, but i want to improve to about this.

thanks.

Copy link
Member

Choose a reason for hiding this comment

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

No i mean this PR idea is awesome because users should be aware of this current incompatibility. If you add the warning, would be happy to merge it for now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay just moment :)

@hagi4u
Copy link
Contributor Author

hagi4u commented Feb 26, 2020

i added warning :)

image

lib/module.js Outdated
if (typeof this.options.head === 'function') {
// eslint-disable-next-line no-console
console.warn('[@nuxtjs/gtm] head is provided as a function which is not supported by this module at the moment. Removing user-provided head.')
this.options.head = typeof this.options.head === 'function' ? {} : this.options.head
Copy link
Member

Choose a reason for hiding this comment

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

I think this typeof check is not necessary anymore (upper-level condition is checking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm fixing.. T0T

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor Author

@hagi4u hagi4u Feb 26, 2020

Choose a reason for hiding this comment

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

am i try to first Open Source PR of my develop life.. please understand to insufficient processes.

Copy link
Member

Choose a reason for hiding this comment

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

NP! Happy to hear you are willing to do more OSS stuff :) Wish to see more contributions by you 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm glad you, i will try to fix and improve more OSS!
thanks

@pi0 pi0 changed the title fix(config): to execute when head property is function on nuxt.config.js fix(module): to execute when head property is function on nuxt.config.js Feb 26, 2020
@pi0 pi0 merged commit 8b37f8d into nuxt-community:master Feb 26, 2020
@pi0 pi0 changed the title fix(module): to execute when head property is function on nuxt.config.js fix(module): warn about head as a function in nuxt config is not supported Feb 26, 2020
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.

None yet

2 participants