yarn-support #488

mattiaerre · 2017-05-24T06:22:50Z

Description

get template by name
fix test

I've added a require by name step before the existing require by path ones; I've also added some small refactoring. I've tested this fix locally and it looks like it works w/ both npm and yarn.

Coverage report

 PASS  __tests__/src/utils/require-template.test.js
  requireTemplate
    ✓ expect type of `requireTemplate` to be function (3ms)
    valid
      ✓ oc-template-handlebars (68ms)
      ✓ oc-template-jade (267ms)
    not valid
      ✓ lodash (35ms)
      ✓ oc-invalid-template (3ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   4 passed, 4 total
Time:        1.432s
Ran all test suites.
---------------------|----------|----------|----------|----------|----------------|
File                 |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
---------------------|----------|----------|----------|----------|----------------|
All files            |       96 |     87.5 |      100 |       96 |                |
 require-template.js |       96 |     87.5 |      100 |       96 |             24 |
---------------------|----------|----------|----------|----------|----------------|

mattiaerre · 2017-05-24T06:24:47Z

src/utils/require-template.js

-    }
-    ocTemplate = require(localTemplate);
-  } catch(err) {
+    ocTemplate = getOcTemplate(template);


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

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 😕

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!

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

😕

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 😢

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

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

the neverending story... 😅

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.

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

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

// cc @matteofigus @nickbalestra

mattiaerre · 2017-06-04T18:24:22Z

@matteofigus @nickbalestra @elboletaire not sure if we decided how to proceed w/ this issue; infinite-loop-loader was the last blocker we had.

mattiaerre · 2017-06-13T03:15:53Z

@matteofigus shall we have a conversation and decide how to proceed w/ this?

// cc @nickbalestra @elboletaire

matteofigus · 2017-06-13T10:39:26Z

@mattiaerre sorry for taking long to reply to this.

Unfortunately requiring by name doesn't work. You want to require the template from the CLI local to the component. Requiring by name requires it local to the CLI, which happens to work currently for the jade/handlebars ones, but that will change soon in favour of taking those two off from the CLI (making the require by name attempt basically not useful).

Now, getting back to the issue. I am not very expert about yarn, but this seems to be a yarn problem to me. There is a package.json, yarn is not installing all the deps during the install, so I would open a issue in their repo rather than here.

matteofigus · 2017-06-13T10:42:03Z

@nickbalestra please can you review what I just said makes sense? lol

matteofigus · 2017-06-13T10:45:16Z

src/utils/require-template.js

+  return require(path);
+};
+
+module.exports = function (template) {
  let ocTemplate;
  const localTemplate = path.join(


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

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

matteofigus · 2017-06-13T13:10:31Z

Ok...I apology for previous comments.
I finally managed to reproduce.
Summarising:
yarn global add oc installs oc into => /Users/mfigus/.nvm/versions/node/v6.10.2/bin/oc
That thing is a symlink to => /Users/mfigus/.config/yarn/global/node_modules/oc/

Now...If I go in that node_modules folder, it's pretty empty. The main reason, is that if I go 1 level down (/Users/mfigus/.config/yarn/global/node_modules/) the dependencies (including infinite-loop-loader and oc-template-handlebars) are there already. So, I believe yarn tries to be smart and doesn't install them, but in the module we actually require it from the oc's node_modules.

So, in conclusion, I believe @mattiaerre's approach in this PR is correct and will fix.
So, I am approving this PR.

matteofigus

@mattiaerre I am ok with the approach.
Before merging:

I prefer not including the yarn lock file. I am ok supporting yarn, but I would remove the lock file
I am not sure about the jest tests here. We have another PR to discuss and I am personally happy about it, but for limiting this PR's scope my preference would be not to use jest. Feel free to disagree...as said, I have mixed feelings, we need to start with something at some point.

elboletaire · 2017-06-13T13:19:16Z

+1 to what @matteofigus said about the yarn.lock file. It's ok to include it in final projects, but not in libraries (exactly how composer.lock works for php projects). So it should not be included in oc.

And I also think that, if we don't currently have testing, we should not merge issues. Let's add a new issue for adding tests. Given the moment, @mattiaerre will be able to cherry-pick the changes from his branch to the new PR.

mattiaerre · 2017-06-16T21:31:41Z

thank you for your feedback @elboletaire I adapted my tests to use mocha and it looks like I've been able to achieve the same result w/o Jest (although no snapshots, unfortunately); apart from that I think that this is a decent refactoring so as long as we are good w/ infinite-loop-loader this PR is over to you for a review and no longer WIP

a quote from @ericelliott

Open source etiquette requires a "when in Rome" attitude. Don't make major changes to tech or style without discussing it first.

source: twitter

// cc @matteofigus @nickbalestra

matteofigus · 2017-06-17T10:43:04Z

Nice quote 👍 It now LGTM. If it's good for you too, @elboletaire, can you merge?

Thanks

elboletaire · 2017-06-17T11:44:34Z

Done @matteofigus. Thanks for the fix @mattiaerre !!

mattiaerre · 2017-06-17T18:07:55Z

@elboletaire thank you!

mattiaerre added 2 commits May 23, 2017 22:52

get template by name

f0a56d1

fix test

f289a5c

mattiaerre requested review from nickbalestra, elboletaire and matteofigus May 24, 2017 06:23

mattiaerre self-assigned this May 24, 2017

mattiaerre commented May 24, 2017

View reviewed changes

mattiaerre mentioned this pull request May 25, 2017

Add toThrowErrorMatchingSnapshot mattiaerre/jest-do-it#2

Closed

matteofigus added the wip label Jun 7, 2017

Merge branch 'master' into yarn-support

ab7e7bb

matteofigus reviewed Jun 13, 2017

View reviewed changes

matteofigus mentioned this pull request Jun 13, 2017

Yarn support #487

Closed

matteofigus requested changes Jun 13, 2017

View reviewed changes

mattiaerre added 4 commits June 16, 2017 13:54

use mocha

66fa535

remove jest

bdc3b1b

consolidate test

035dae2

no destructuring

085b4e6

mattiaerre changed the title ~~[DO NOT MERGE] yarn-support~~ yarn-support Jun 16, 2017

mattiaerre removed the wip label Jun 16, 2017

matteofigus approved these changes Jun 17, 2017

View reviewed changes

elboletaire approved these changes Jun 17, 2017

View reviewed changes

elboletaire merged commit 3ca8ffd into master Jun 17, 2017

matteofigus deleted the yarn-support branch June 17, 2017 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

yarn-support #488

yarn-support #488

mattiaerre commented May 24, 2017

mattiaerre May 24, 2017

elboletaire May 24, 2017

mattiaerre May 24, 2017

elboletaire May 24, 2017

mattiaerre May 24, 2017

elboletaire May 24, 2017

mattiaerre May 24, 2017

elboletaire May 24, 2017

mattiaerre May 24, 2017

mattiaerre commented Jun 4, 2017

mattiaerre commented Jun 13, 2017

matteofigus commented Jun 13, 2017

matteofigus commented Jun 13, 2017

matteofigus Jun 13, 2017

matteofigus Jun 13, 2017

matteofigus commented Jun 13, 2017

matteofigus left a comment •

edited

Loading

elboletaire commented Jun 13, 2017

mattiaerre commented Jun 16, 2017

matteofigus commented Jun 17, 2017

elboletaire commented Jun 17, 2017 •

edited

Loading

mattiaerre commented Jun 17, 2017

yarn-support #488

yarn-support #488

Conversation

mattiaerre commented May 24, 2017

Description

Coverage report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattiaerre commented Jun 4, 2017

mattiaerre commented Jun 13, 2017

matteofigus commented Jun 13, 2017

matteofigus commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteofigus commented Jun 13, 2017

matteofigus left a comment • edited Loading

Choose a reason for hiding this comment

elboletaire commented Jun 13, 2017

mattiaerre commented Jun 16, 2017

matteofigus commented Jun 17, 2017

elboletaire commented Jun 17, 2017 • edited Loading

mattiaerre commented Jun 17, 2017

matteofigus left a comment •

edited

Loading

elboletaire commented Jun 17, 2017 •

edited

Loading