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

feat: warn if imported variable value name not valid #2

Merged
merged 13 commits into from
Jan 15, 2019
38 changes: 32 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,17 @@ const path = require('path');
const BroccoliFilter = require('broccoli-persistent-filter');
const md5Hex = require('md5-hex');

const IMPORT_PATTERN = /\{\{\s*import (\w+) from ['"]([^'"]+)['"]\s*\}\}/gi;
const IMPORT_PATTERN = /\{\{\s*import\s+([^\s]+)\s+from\s+['"]([^'"]+)['"]\s*\}\}/gi;

function isValidVariableName(name) {
if (!(/^[A-Za-z0-9]+$/.test(name))) {
return false;
}
if (name.charAt(0).toUpperCase() !== name.charAt(0)) {
return false;
}
return true;
}
class TemplateImportProcessor extends BroccoliFilter {

constructor(inputNode, options = {}) {
Expand Down Expand Up @@ -42,15 +51,32 @@ class TemplateImportProcessor extends BroccoliFilter {
let imports = [];
let rewrittenContents = contents.replace(IMPORT_PATTERN, (_, localName, importPath) => {
if (importPath.startsWith('.')) {
importPath = path.resolve(relativePath, '..', importPath);
importPath = path.relative(this.options.root, importPath);
importPath = path.resolve(relativePath, '..', importPath).split(path.sep).join('/');
lifeart marked this conversation as resolved.
Show resolved Hide resolved
importPath = path.relative(this.options.root, importPath).split(path.sep).join('/');
}
imports.push({ localName, importPath });
imports.push({ localName, importPath, isLocalNameValid: isValidVariableName(localName) });
return '';
});

let header = imports.map(({ importPath, localName }) => {
return `{{#let (component '${ importPath }') as |${ localName }|}}`;
let header = imports.map(({ importPath, localName, isLocalNameValid }) => {
const warnPrefix = 'ember-template-component-import: ';
const abstractWarn = `${warnPrefix} Allowed import variable names - CamelCased strings, like: FooBar, TomDale`;
const componentWarn = `
${warnPrefix}Warning!
in file: "${relativePath}"
subject: "${localName}" is not allowed as Variable name for Template import.`;
const warn = isLocalNameValid ? '' : `
<pre data-test-name="${localName}">${componentWarn}</pre>
<pre data-test-global-warn="${localName}">${abstractWarn}</pre>
`;
if (!isLocalNameValid) {
this._console.log(componentWarn);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we throw an error here, does it kill the ember server? If not, I'd say lean towards throwing here. If it's not a valid identifier, it cannot work, so I don't see a strong use case for letting it off as a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - but I imagine that is probably harder to test for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, can't figure out how to test it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added throwing for all cases, instead of testing.
if we found incorrect name in path !== 'dummy/pods/application/template.hbs' we throw.
else - we log error (and able to test throwed message)

if (relativePath !== 'dummy/pods/application/template.hbs') {
// don't throw on 'dummy/pods/application/template.hbs' (test template)
throw new Error(componentWarn);
}
}
return `${warn}{{#let (component '${ importPath }') as |${ localName }|}}`;
}).join('');
let footer = imports.map(() => `{{/let}}`).join('');

Expand Down
21 changes: 20 additions & 1 deletion tests/acceptance/import-test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { module, test } from 'qunit';
import { visit, find } from '@ember/test-helpers';
import { visit, find, findAll } from '@ember/test-helpers';
import { setupApplicationTest } from 'ember-qunit';

module('Acceptance | import', function(hooks) {
Expand All @@ -9,5 +9,24 @@ module('Acceptance | import', function(hooks) {
await visit('/');
assert.equal(find('.global-button').innerText, "I'm a globally referenced button");
assert.equal(find('.local-button').innerText, "I'm a locally referenced button");
const incorrectlyCamelCasedAbsoluteImportText = find('[data-test-name="incorrectlyCamelCasedAbsoluteImport"]').innerText;
const incorrectlyCamelCasedRelativeImportText = find('[data-test-name="incorrectlyCamelCasedRelativeImport"]').innerText;
const Incorrectly_Snake_Cased_Absolute_ImportText = find('[data-test-name="Incorrectly_Snake_Cased_Absolute_Import"]').innerText;
const Incorrectly_Snake_Cased_Relative_ImportText = find('[data-test-name="Incorrectly_Snake_Cased_Relative_Import"]').innerText;

assert.equal(incorrectlyCamelCasedAbsoluteImportText.includes('dummy/pods/application/template.hbs'), true);
assert.equal(incorrectlyCamelCasedAbsoluteImportText.includes('"incorrectlyCamelCasedAbsoluteImport"'), true);
assert.equal(incorrectlyCamelCasedRelativeImportText.includes('dummy/pods/application/template.hbs'), true);
assert.equal(incorrectlyCamelCasedRelativeImportText.includes('"incorrectlyCamelCasedRelativeImport"'), true);


assert.equal(Incorrectly_Snake_Cased_Absolute_ImportText.includes('dummy/pods/application/template.hbs'), true);
assert.equal(Incorrectly_Snake_Cased_Absolute_ImportText.includes('"Incorrectly_Snake_Cased_Absolute_Import"'), true);
assert.equal(Incorrectly_Snake_Cased_Relative_ImportText.includes('dummy/pods/application/template.hbs'), true);
assert.equal(Incorrectly_Snake_Cased_Relative_ImportText.includes('"Incorrectly_Snake_Cased_Relative_Import"'), true);

assert.equal(findAll('[data-test-global-warn]').length, 4);
assert.equal(findAll('.global-button').length, 3);
assert.equal(findAll('.local-button').length, 4);
});
});
14 changes: 12 additions & 2 deletions tests/dummy/app/pods/application/template.hbs
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
{{import Button from "ui/button"}}
{{import LocalButton from "./button"}}
{{import SecondButton from "./button"}}
{{import ButtonWithNumber5 from "./button"}};
{{import incorrectlyCamelCasedAbsoluteImport from "ui/button"}}
{{import incorrectlyCamelCasedRelativeImport from "./button"}}
{{import Incorrectly_Snake_Cased_Absolute_Import from "ui/button"}}
{{import Incorrectly_Snake_Cased_Relative_Import from "./button"}}

<Button>I'm a globally referenced button</Button>
<LocalButton>I'm a locally referenced button</LocalButton>
<SecondButton>I'm a locally referenced button</SecondButton>
<ButtonWithNumber5>I'm ButtonWithNumber5InComponentName</ButtonWithNumber5>
<incorrectlyCamelCasedAbsoluteImport>I'm a dirty named global button</incorrectlyCamelCasedAbsoluteImport>
<incorrectlyCamelCasedRelativeImport>I'm a dirty named global button</incorrectlyCamelCasedRelativeImport>
<Incorrectly_Snake_Cased_Absolute_Import>I'm a underscored global button</Incorrectly_Snake_Cased_Absolute_Import>
<Incorrectly_Snake_Cased_Relative_Import>I'm a underscored local button</Incorrectly_Snake_Cased_Relative_Import>
31 changes: 31 additions & 0 deletions tests/dummy/app/pods/different-imports/template.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// spaced
{{ import Button from "ui/button" }}
// tabbed
{{ import SecondButton from "application/button" }}
// mixed
{{ import ButtonButton from "ui/button" }}
// newlined
{{import
SecondButtonSecondButton
from
"application/button"
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I didn't think of multi-line ...

// newlined mixed
{{import
ButtonButtonButton
from
"ui/button"
}}
// newlined mixed with tabs and spaces
{{ import
SecondButtonSecondButtonSecondButton
from
"application/button"
}}

<Button>b1</Button>
<SecondButton>b1</SecondButton>
<ButtonButton>b3</ButtonButton>
<SecondButtonSecondButton>b4</SecondButtonSecondButton>
<SecondButtonSecondButtonSecondButton>b5</SecondButtonSecondButtonSecondButton>
<ButtonButtonButton>b6</ButtonButtonButton>
13 changes: 13 additions & 0 deletions tests/integration/pods/formatted-import-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render, findAll } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';

module('Integration | Component | different-imports', function(hooks) {
setupRenderingTest(hooks);

test('it can handle spaces, tabbed, multilined imports', async function(assert) {
await render(hbs`{{different-imports}}`);
assert.equal(findAll('button').length, 6);
});
});