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

yarn-support #488

Merged
merged 7 commits into from
Jun 17, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,6 @@ src/components/base-component-handlebars/_package
src/components/base-component-jade/_package

test/fixtures/test.tar.gz
test/fixtures/targz-test
test/fixtures/targz-test

coverage
23 changes: 23 additions & 0 deletions __tests__/src/utils/__snapshots__/require-template.test.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`requireTemplate not valid lodash 1`] = `"Error requiring oc-template: \\"lodash\\" is not a valid oc-template"`;

exports[`requireTemplate not valid oc-invalid-template 1`] = `"Error requiring oc-template: \\"oc-invalid-template\\" not found"`;

exports[`requireTemplate valid oc-template-handlebars 1`] = `
Object {
"compile": [Function],
"getCompiledTemplate": [Function],
"getInfo": [Function],
"render": [Function],
}
`;

exports[`requireTemplate valid oc-template-jade 1`] = `
Object {
"compile": [Function],
"getCompiledTemplate": [Function],
"getInfo": [Function],
"render": [Function],
}
`;
44 changes: 44 additions & 0 deletions __tests__/src/utils/require-template.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
const requireTemplate = require('../../../src/utils/require-template');

describe('requireTemplate', () => {
test('expect type of `requireTemplate` to be function', () => {
expect(typeof requireTemplate).toBe('function');
});

describe('valid', () => {
const scenarios = [
{ name: 'oc-template-handlebars' },
{ name: 'oc-template-jade' }
];

scenarios.forEach(({ name }) => {
test(name, () => {
const template = requireTemplate(name);

['compile', 'getCompiledTemplate', 'getInfo', 'render']
.forEach((method) => {
expect(template).toHaveProperty(method);
});
expect(template).toMatchSnapshot();
});
});
});

describe('not valid', () => {
const scenarios = [
{ name: 'lodash' },
{ name: 'oc-invalid-template' }
];

scenarios.forEach(({ name }) => {
test(name, () => {
const sut = () => {
requireTemplate(name);
};

expect(sut).toThrow();
expect(sut).toThrowErrorMatchingSnapshot();
});
});
});
});
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"husky": "^0.13.4",
"injectr": "0.5.1",
"jasmine-core": "^2.5.1",
"jest": "^20.0.3",
"karma": "^1.3.0",
"karma-jasmine": "^1.0.2",
"karma-phantomjs-launcher": "^1.0.2",
Expand Down
27 changes: 16 additions & 11 deletions src/utils/require-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@ function isValidTemplate(template) {
);
}

module.exports = function(template) {
const getOcTemplate = (path) => {
if (require.cache && !!require.cache[path]) {
delete require.cache[path];
}
return require(path);
};

module.exports = function (template) {
let ocTemplate;
const localTemplate = path.join(
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 we need to understand, in case of yarn install, why this doesn't work. The oc-template-jade module must be there if yarn installed everything correctly, if it is not we need to find out where it is

Copy link
Member

Choose a reason for hiding this comment

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

Same for the infinite-loop-loader. It should be in the node_modules.

__dirname,
Expand All @@ -28,23 +35,21 @@ module.exports = function(template) {
const relativeTemplate = path.resolve('.', 'node_modules', template);

try {
if (require.cache && !!require.cache[localTemplate]) {
delete require.cache[localTemplate];
}
ocTemplate = require(localTemplate);
ocTemplate = getOcTemplate(template);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only "new" code I've added that tries to fix the yarn issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry about my ignorance, but how can that change affect over the yarn installation behavior? I only see you moved the logic to a function 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi,
Yes I moved the logic into a function but I've also added an additional check of getting the template by name (line 35).
Try to do this: install OC globally w/ yarn and make sure your publish doesn't work. Then replace require-template.js in the yarn OC global folder w/ the file I've changed and see if that fixes. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just tried it, and I can't publish due to this: npm/npmlog#48

imatge

😕

Copy link
Member Author

Choose a reason for hiding this comment

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

this is another problem; you need to rm -rf node_modules first, it is a known issue w/ npmlog when switching between npm and yarn 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I was doing it now... and then I face with this...

imatge

After seeing the error... I thought that maybe your changes now could allow me to publish from the widget folder itself, so I tried:

imatge

the neverending story... 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

infinite-loop-loader tells us everything hehehe 😊 feel free to collaborate on this branch; I will not be able to follow this issue as I'll be offline for a couple of days but either @matteofigus or @nickbalestra should be able to have a look at this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After manually installing infinite-loop-loader (ofc only for this test) everything works as expected! 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

very good; we are getting there 😉 thanks for your help!

// cc @matteofigus @nickbalestra

} catch (err) {
try {
if (require.cache && !!require.cache[relativeTemplate]) {
delete require.cache[relativeTemplate];
}
ocTemplate = require(relativeTemplate);
ocTemplate = getOcTemplate(localTemplate);
} catch (err) {
throw format(templateNotFound, template);
try {
ocTemplate = getOcTemplate(relativeTemplate);
} catch (err) {
throw new Error(format(templateNotFound, template));
}
}
}

if (!isValidTemplate(ocTemplate)) {
throw format(templateNotValid, template);
throw new Error(format(templateNotValid, template));
}

return ocTemplate;
Expand Down
2 changes: 1 addition & 1 deletion test/unit/cli-domain-package-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('cli : domain : package-template', () => {
});

it('should show error', () => {
expect(error).to.equal('template.wha compilation failed - Error requiring oc-template: "whazaaa" not found');
expect(error).to.equal('template.wha compilation failed - Error: Error requiring oc-template: "whazaaa" not found');
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/registry-domain-repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ describe('registry : domain : repository', () => {
try {
Repository(conf);
} catch (err) {
expect(err).to.equal('Error requiring oc-template: "oc-template-react" not found');
expect(err.message).to.equal('Error requiring oc-template: "oc-template-react" not found');
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/utils-require-templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('utils : require-template', () => {
try {
requireTemplate('handlebars');
} catch (e) {
expect(e).to.equal('Error requiring oc-template: "handlebars" is not a valid oc-template');
expect(e.message).to.equal('Error requiring oc-template: "handlebars" is not a valid oc-template');
}
});
});
Loading