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(config): typo in unsafeInlineCompatibility name #6583

Merged
merged 4 commits into from
Oct 19, 2019

Conversation

PedroD
Copy link

@PedroD PedroD commented Oct 17, 2019

Replacing all occurrences of unsafeInlineCompatiblity to unsafeInlineCompatibility (notice the missing "i" in "Compatibility".

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)
    Edit: Removed the [x] from "Breaking change", as it is now a non-breaking typo fix.

Description

Just fixing a minor typo that can create confusion when typing this property (correctly) from memory. This can lead to wasted time in debugging a wrong property name.

Replacing all occurrences of unsafeInlineCompatiblity to unsafeInlineCompatibility.

Note: This can cause all apps using the old property name to stop working!

Checklist:

@PedroD PedroD changed the title Replacing all occurrences of unsafeInlineCompatiblity to unsafeInline… Fixing a small typo on SSR CSP property name. Oct 17, 2019
@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #6583 into dev will increase coverage by 31.13%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##              dev    #6583       +/-   ##
===========================================
+ Coverage   64.73%   95.86%   +31.13%     
===========================================
  Files          78       78               
  Lines        2705     2709        +4     
  Branches      699      700        +1     
===========================================
+ Hits         1751     2597      +846     
+ Misses        759       97      -662     
+ Partials      195       15      -180
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 64.67% <20%> (-0.06%) ⬇️
#unit 92.32% <100%> (?)
Impacted Files Coverage Δ
packages/vue-renderer/src/renderers/ssr.js 94.02% <100%> (+26.86%) ⬆️
packages/config/src/options.js 100% <100%> (+24.7%) ⬆️
packages/vue-renderer/src/renderers/spa.js 87.87% <0%> (+3.03%) ⬆️
packages/webpack/src/config/client.js 98.07% <0%> (+3.84%) ⬆️
packages/webpack/src/utils/perf-loader.js 100% <0%> (+5.26%) ⬆️
packages/webpack/src/config/base.js 92.07% <0%> (+5.94%) ⬆️
packages/utils/src/lang.js 100% <0%> (+6.66%) ⬆️
packages/core/src/module.js 98.33% <0%> (+10%) ⬆️
packages/utils/src/context.js 100% <0%> (+11.11%) ⬆️
packages/cli/src/commands/generate.js 91.3% <0%> (+13.04%) ⬆️
... and 39 more

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 99aba87...48f0fa6. Read the comment docs.

@PedroD PedroD changed the title Fixing a small typo on SSR CSP property name. fix(ssr): Fixing a small typo on SSR CSP property name. Oct 17, 2019
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

This is a breaking change for users who are using unsafeInlineCompatiblity, we can change it in major release or normalize unsafeInlineCompatiblity to unsafeInlineCompatiblity with a warning message

@TheAlexLichter
Copy link
Member

@PedroD Example how you could do that: https://github.com/nuxt/nuxt.js/pull/4674/files

@PedroD
Copy link
Author

PedroD commented Oct 18, 2019

Ok, allow me some time, I'll try to implement this.

…rder to avoid breaking-changes (displaying an alert), but also allowing the correct name `unsafeInlineCompatibility` to be used.

Introduced TODO comments/notes that should be resolved before the release of Nuxt 3.
@PedroD
Copy link
Author

PedroD commented Oct 18, 2019

Please take a look if the changes I made are ok.

So Nuxt should allow the usage of both old and new property names. When the developer uses the old name, an alert message appears.

I created tests to make sure both names are supported, thus making this PR non-breaking.

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

LGTM,could you open a pr for changing doc as well? Thanks🙏

@PedroD
Copy link
Author

PedroD commented Oct 18, 2019

@clarkdo I think the doc was already updated accordingly, please take a look here: nuxt/docs#1650

How can I link this PR to the existing doc PR?

@clarkdo
Copy link
Member

clarkdo commented Oct 18, 2019

Great, thanks

@PedroD
Copy link
Author

PedroD commented Oct 18, 2019

Hmmm, there seems to be a linter error on file examples/with-sockets/io/index.js

image

This file comes from the merge result (wasn't modified on this PR). Is it an error caused by a previous unrelated commit?

What is the standard procedure in this situation? Shall I try to fix these on my PR?

@PedroD
Copy link
Author

PedroD commented Oct 18, 2019

I found that the problematic file comes from this PR (#6586).

The linter also failed in there: https://circleci.com/gh/nuxt/nuxt.js/56629?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

I can try and fix the issue still on the scope of this PR (if I still have permissions after the merge attempt), or this should be done in the original PR? Or in a new PR at all?

@clarkdo
Copy link
Member

clarkdo commented Oct 18, 2019

Fixed

@pi0 pi0 changed the title fix(ssr): Fixing a small typo on SSR CSP property name. fix(config): typo in unsafeInlineCompatibility name Oct 19, 2019
@pi0 pi0 merged commit 257ae22 into nuxt:dev Oct 19, 2019
@pi0 pi0 mentioned this pull request Oct 21, 2019
@pi0 pi0 mentioned this pull request Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants