update substitution placeholder message in babel-template #7255

thescientist13 · 2018-01-22T23:41:34Z

Q	A
Fixed Issues?	helps resolve #7035
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR	No
Any Dependency Changes?	No
License	MIT

Updated babel-template placeholder text for missing substitution per suggestion here

thescientist13 · 2018-01-22T23:45:28Z

packages/babel-template/src/populate.js

-        throw new Error(`No substitution given for "${placeholder.name}"`);
+        throw new Error(`Error: No substitution given for "NODE_ENV". If this is not meant to be a
+          placeholder you may want to consider passing one of the following options to babel-template:
+          - { placeholderPattern: false, placeholderWhitelist: new Set(['NODE']) }


Not sure if
new Set(['NODE']

should be this ?
new Set(['NODE_ENV']

The error message should continue using placeholder.name, don't think we want to say NODE_ENV for every error :)

@existentialism
Ah... of course!

thescientist13 · 2018-01-22T23:56:50Z

Working on fixing the test, just getting a little hung up on formatting / linting for a multiline string in the test, example (I'm using Webstorm for IDE if it matters)

$ TEST_ONLY=babel-template make test
./node_modules/.bin/eslint scripts packages codemods *.js --format=codeframe --rulesdir="./scripts/eslint_rules"
error: Delete `⏎········` (prettier/prettier) at packages/babel-template/src/populate.js:21:46:
  19 |           placeholder you may want to consider passing one of the following options to babel-template:
  20 |           - { placeholderPattern: false, placeholderWhitelist: new Set(['NODE']) }
> 21 |           - { placeholderPattern: /^NODE$/ }`
     |                                              ^
  22 |         );
  23 |       }
  24 |     });

note: the test didn't fail locally for me running TEST_ONLY=babel-template make test, not sure if that is expected or not. (maybe not since it is dealing with NODE_ENV?)

maazadeeb · 2018-01-23T00:26:20Z

Try running make fix to fix all the problems that can be fixed automatically. All errors reported by prettier are auto-fixable.

abiduzz420 · 2018-01-23T00:30:25Z

@thescientist13 You need to modify the expected error message here https://github.com/babel/babel/blob/master/packages/babel-template/test/index.js#L130
Hope this helps.

thescientist13 · 2018-01-23T00:31:36Z

thank you for that tip @maaz93 !! The console output showed --fix so I was sad that it wasn't working for me

3 errors potentially fixable with the --fix option.

Now it's working! 🎉

thescientist13 · 2018-01-23T00:32:19Z

thanks @abiduzz420, that's what I'm working on now 👍

thescientist13 · 2018-01-23T00:58:56Z

think I got it now 🤞

babel-bot · 2018-01-23T01:08:40Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6710/

thescientist13 · 2018-01-23T01:08:45Z

packages/babel-template/src/populate.js

+        throw new Error(`Error: No substitution given for "${
+          placeholder.name
+        }". If this is not meant to be a
+          placeholder you may want to consider passing one of the following options to babel-template:


this formatting was put in by prettier. maybe it expects { to always be the start of an object and thus need a line break after?

let me know if this 🆗

Template literals, the ``, preserve spaces and new lines. You'll need to construct your string something like the way it's done here

babel/packages/babel-core/src/transformation/normalize-file.js

Line 82 in 180eda3

`You appear to be using an async codegen plugin, ` +

Otherwise, the error message will have unnecessary indentation.

@maaz93
It seems to keep complaining about that {

if ( !Object.prototype.hasOwnProperty.call(replacements, placeholder.name) ) { throw new Error( `Error: No substitution given for "${placeholder.name}". If this is not meant ` + `to be a placeholder you may want to consider passing one of the following options to babel-template:` + `- { placeholderPattern: false, placeholderWhitelist: new Set(['${placeholder.name}'])}` + `- { placeholderPattern: /^${placeholder.name}$/ }` ); }

error: Replace `··`Error:·No·substitution·given·for·"${placeholder.name` with ``Error:·No·substitution·given·for·"${⏎············placeholder.name⏎··········` (prettier/prettier) at packages/babel-template/src/populate.js:19:11: 17 | ) { 18 | throw new Error( > 19 | `Error: No substitution given for "${placeholder.name}". If this is not meant ` + | ^ 20 | `to be a placeholder you may want to consider passing one of the following options to babel-template:` + 21 | `- { placeholderPattern: false, placeholderWhitelist: new Set(['${placeholder.name}'])}` + 22 | `- { placeholderPattern: /^${placeholder.name}$/ }`

Alright. This seems a little tricky. I formatted it here to work fine:

const placeholderName = placeholder.name; throw new Error( `Error: No substitution given for "${placeholderName}". If this is not meant to be a placeholder ` + `you may want to consider passing one of the following options to babel-template:` + `- { placeholderPattern: false, placeholderWhitelist: new Set(['${placeholderName}'])}` + `- { placeholderPattern: /^${placeholderName}$/ }`, );

It was complaining about printWidth and the { placement. I've overcome the { by not having a property access, rather storing it in placeholderName. Also, I changed number of characters per line in one of the lines to meet the allowed printWidth. Let me know if this works

maazadeeb · 2018-01-23T14:17:18Z

It's generally recommended to run make test once locally, after you make a local commit. This will give you an opportunity to see if any tests are failing or not on your system, instead of waiting for the CI. Were you able to figure out why tests in your last commit failed?

thescientist13 · 2018-01-23T16:43:11Z

@maaz93
It looks like it's because of the formatting still (but because the output differs because of how the test / populatePlaceholders are constructing the string.

I have tried again with using `` for both still getting test failures on the expected vs actual
populate.js

        const placeholderName = placeholder.name;

        throw new Error(
          `Error: No substitution given for "${placeholderName}". If this is not meant to be a 
            placeholder you may want to consider passing one of the following options to babel-template:
            - { placeholderPattern: false, placeholderWhitelist: new Set(['${placeholderName}'])}
            - { placeholderPattern: /^${placeholderName}$/ }`,
        );

index.js

      }).to.throw(
        Error,
        `Error: No substitution given for "ANOTHER_ID". If this is not meant to be a
          placeholder you may want to consider passing one of the following options to babel-template:
          - { placeholderPattern: false, placeholderWhitelist: new Set(['ANOTHER_ID']) }
          - { placeholderPattern: /^ANOTHER_ID$/ }`,
      );

Locally with both implementations seemingly the same, I am seeing this test failure occuring

 1) babel-template string-based should throw if placeholders are not given explicit values:

      AssertionError: expected [Function] to throw error including 'Error: No substitution given for "ANOTHER_ID". If this is not meant to be a\n          placeholder you may want to consider passing one of the following options to babel-template:\n          - { placeholderPattern: false, placeholderWhitelist: new Set([\'ANOTHER_ID\']) }\n          - { placeholderPattern: /^ANOTHER_ID$/ }' but got 'Error: No substitution given for "ANOTHER_ID". If this is not meant to be a placeholderyou may want to consider passing one of the following options to babel-template:- { placeholderPattern: false, placeholderWhitelist: new Set([\'ANOTHER_ID\'])}- { placeholderPattern: /^ANOTHER_ID$/ }'
      + expected - actual

      -Error: No substitution given for "ANOTHER_ID". If this is not meant to be a placeholderyou may want to consider passing one of the following options to babel-template:- { placeholderPattern: false, placeholderWhitelist: new Set(['ANOTHER_ID'])}- { placeholderPattern: /^ANOTHER_ID$/ }
      +Error: No substitution given for "ANOTHER_ID". If this is not meant to be a
      +          placeholder you may want to consider passing one of the following options to babel-template:
      +          - { placeholderPattern: false, placeholderWhitelist: new Set(['ANOTHER_ID']) }
      +          - { placeholderPattern: /^ANOTHER_ID$/ }

nveenjain · 2018-01-24T07:10:27Z

packages/babel-template/test/index.js

-      }).to.throw(Error, 'No substitution given for "ANOTHER_ID"');
+      }).to.throw(
+        Error,
+        `Error: No substitution given for "ANOTHER_ID". If this is not meant to be a


ES6 template literals take raw string, so if you have multi-line string, then it will automatically add new Line character there. However, when you use + (with strings), you are actually doing concatenation and not putting any new Line character there. I would suggest you to convert both of them into template literals, that would automatically add new line character in both. Hence resolving test!

thanks @nveenjain
I forgot to include it in my comment, but locally both implementations are seemingly matching to me in terms of implementation, yet still getting a test failure on string construction. (I've since updated my comment to show everything)

I can push my changes if it helps.

this one is still constructed with multiline template string, you just have to adjust this one so it is constructed like the other one (the other one is correct)

@Andarist
Do you mean in this PR? Yes I understand they are different and I know why they are incompatible, but if you see the updated link from my comment above, locally they seem identical to me. So still struggling with the difference between this

const placeholderName = placeholder.name; throw new Error( `Error: No substitution given for "${placeholderName}". If this is not meant to be a placeholder you may want to consider passing one of the following options to babel-template: - { placeholderPattern: false, placeholderWhitelist: new Set(['${placeholderName}'])} - { placeholderPattern: /^${placeholderName}$/ }`, );

and this

}).to.throw( Error, `Error: No substitution given for "ANOTHER_ID". If this is not meant to be a placeholder you may want to consider passing one of the following options to babel-template: - { placeholderPattern: false, placeholderWhitelist: new Set(['ANOTHER_ID']) } - { placeholderPattern: /^ANOTHER_ID$/ }`, );

🤒

notice that in the source file (when throwing) the string is created (in the PR) with concatenation (+) and in tests it's defined with multiline template literal

This:

const str1 = `first line` + `second line`

and this:

const str2 = `first line second line`

are not equivalents.

str1 === 'first linesecond line' str2 === 'first line\n\t\t\t\t\t\t\t\t\t\t\t\tsecond line'

@Andarist
Understood that is what is causing the failure in this PR specifically, but my comment was just trying to demonstrate what I was trying locally to fix the failure so as to avoid pushing intentionally failing commits.

I suppose that keeping my local work local is causing more confusion, so I will just push my latest changes.

Appreciate you helping out, sorry for any confusion, hopefully the issue I'm seeing will be better highlighted in a few moments.

@thescientist13 the difference is an extra space after "a" in your input vs output and from travis, as well as the subsequent whitespace:

'Error: No substitution given for "ANOTHER_ID". If this is not meant to be a\n placeholder you may want to consider passing one of the following options to babel-template:\n - { placeholderPattern: false, placeholderWhitelist: new Set([\'ANOTHER_ID\']) }\n - { placeholderPattern: /^ANOTHER_ID$/ }'

vs

'Error: No substitution given for "ANOTHER_ID". If this is not meant to be a \n placeholder you may want to consider passing one of the following options to babel-template:\n - { placeholderPattern: false, placeholderWhitelist: new Set([\'ANOTHER_ID\'])}\n - { placeholderPattern: /^ANOTHER_ID$/ }'`

Thanks @existentialism ! I think part of the issue is that what I see in CircleCI is different output than what I see locally. Will need to see if I need to align my local environment

OSX: 10.13.2

node: 6.9.1

npm: 5.6.0

I think my next goal is to make my local try and match CI.

@thescientist13 i pushed a change to your branch that should fix it, sorry for all the trouble digging into it!

wow, thanks @existentialism !!

I'll certainly never underestimate a string again... 😌

Andarist · 2018-01-24T21:04:28Z

packages/babel-template/src/populate.js

+        const placeholderName = placeholder.name;
+
+        throw new Error(
+          `Error: No substitution given for "${placeholderName}". If this is not meant to be a


multiline string like this will look weirdly in the console, personally I would prefer using regular concatenation for this

fair point and appreciate your assistance / feedback.

String concatenation had been suggested before, but then I got too focused on the struggle with template string to step back and consider the alternatives and wanting to stick with the existing convention.

Lesson learned!

existentialism

LGTM

hzoo · 2018-01-30T16:54:18Z

Thanks for sticking with this - we should use @babel/template instead of babel-template now but I'l just fix it up in another commit

)

linting passing

f0e9c6b

thescientist13 changed the title ~~linting passing~~ update substitution placeholder message in babel-template Jan 22, 2018

thescientist13 commented Jan 22, 2018

View reviewed changes

Owen Buckley added 3 commits January 22, 2018 19:41

updating test

ba398e4

update template with placeholder

7c12a8e

fix failing test

c32b60e

thescientist13 commented Jan 23, 2018

View reviewed changes

fix formatting

c0f683f

nveenjain suggested changes Jan 24, 2018

View reviewed changes

thescientist13 and others added 3 commits January 24, 2018 13:46

update to use template literal syntax

418cf79

remove extra space

06ee19c

fix

229a70d

Andarist reviewed Jan 24, 2018

View reviewed changes

existentialism added PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Polish 💅 A type of pull request used for our changelog categories pkg: template and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jan 30, 2018

existentialism approved these changes Jan 30, 2018

View reviewed changes

hzoo merged commit 2185256 into babel:master Jan 30, 2018

aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018

update substitution placeholder message in @babel/template (babel#7255

ee49f64

)

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 2, 2018

lock bot locked as resolved and limited conversation to collaborators May 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update substitution placeholder message in babel-template #7255

update substitution placeholder message in babel-template #7255

thescientist13 commented Jan 22, 2018 •

edited

Loading

thescientist13 Jan 22, 2018

existentialism Jan 23, 2018

thescientist13 Jan 23, 2018

thescientist13 commented Jan 22, 2018 •

edited

Loading

maazadeeb commented Jan 23, 2018

abiduzz420 commented Jan 23, 2018

thescientist13 commented Jan 23, 2018 •

edited

Loading

thescientist13 commented Jan 23, 2018 •

edited

Loading

thescientist13 commented Jan 23, 2018

babel-bot commented Jan 23, 2018 •

edited

Loading

thescientist13 Jan 23, 2018 •

edited

Loading

maazadeeb Jan 23, 2018

thescientist13 Jan 23, 2018 •

edited

Loading

maazadeeb Jan 23, 2018

maazadeeb commented Jan 23, 2018

thescientist13 commented Jan 23, 2018 •

edited

Loading

nveenjain Jan 24, 2018

thescientist13 Jan 24, 2018

Andarist Jan 24, 2018

thescientist13 Jan 24, 2018 •

edited

Loading

Andarist Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading

existentialism Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading

existentialism Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018

Andarist Jan 24, 2018

thescientist13 Jan 24, 2018 •

edited

Loading

existentialism left a comment

hzoo commented Jan 30, 2018

update substitution placeholder message in babel-template #7255

update substitution placeholder message in babel-template #7255

Conversation

thescientist13 commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 commented Jan 22, 2018 • edited Loading

maazadeeb commented Jan 23, 2018

abiduzz420 commented Jan 23, 2018

thescientist13 commented Jan 23, 2018 • edited Loading

thescientist13 commented Jan 23, 2018 • edited Loading

thescientist13 commented Jan 23, 2018

babel-bot commented Jan 23, 2018 • edited Loading

thescientist13 Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maazadeeb commented Jan 23, 2018

thescientist13 commented Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Andarist Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

thescientist13 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

existentialism Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

thescientist13 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

existentialism Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thescientist13 Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

existentialism left a comment

Choose a reason for hiding this comment

hzoo commented Jan 30, 2018

thescientist13 commented Jan 22, 2018 •

edited

Loading

thescientist13 commented Jan 22, 2018 •

edited

Loading

thescientist13 commented Jan 23, 2018 •

edited

Loading

thescientist13 commented Jan 23, 2018 •

edited

Loading

babel-bot commented Jan 23, 2018 •

edited

Loading

thescientist13 Jan 23, 2018 •

edited

Loading

thescientist13 Jan 23, 2018 •

edited

Loading

thescientist13 commented Jan 23, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading

Andarist Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading

existentialism Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading

existentialism Jan 24, 2018 •

edited

Loading

thescientist13 Jan 24, 2018 •

edited

Loading