env.NODE_PATH support in Storyshots #2873

igor-dv · 2018-01-30T14:54:50Z

Issue: #2838

What I did

I've added NODE_PATH support in the mocked "require.context"

How to test

I have no idea how to add a working example into the monorepo... I've tested it with the repo from the issue.

codecov · 2018-01-30T22:08:17Z

Codecov Report

Merging #2873 into master will increase coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2873      +/-   ##
==========================================
+ Coverage   35.91%   35.92%   +<.01%     
==========================================
  Files         428      428              
  Lines        9483     9486       +3     
  Branches      973      993      +20     
==========================================
+ Hits         3406     3408       +2     
+ Misses       5417     5382      -35     
- Partials      660      696      +36

Impacted Files	Coverage Δ
addons/storyshots/src/require_context.js	`91.66% <75%> (-2.28%)`	⬇️
...-native/src/preview/components/OnDeviceUI/index.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/index.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Date/index.js	`23.52% <0%> (ø)`	⬆️
lib/components/src/table/cell.js	`65.21% <0%> (ø)`	⬆️
lib/codemod/src/transforms/update-addon-info.js	`50% <0%> (ø)`	⬆️
addons/a11y/src/components/Report/Tags.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/containers/search_box.js	`23.52% <0%> (ø)`	⬆️
addons/knobs/src/react/WrapStory.js	`49.42% <0%> (ø)`	⬆️
addons/actions/src/lib/reviver.js	`58.33% <0%> (ø)`	⬆️
... and 47 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 09d03ec...5d3e6eb. Read the comment docs.

tmeasday · 2018-01-30T23:38:04Z

@igor-dv would it not make more sense to not call path.resolve() if the root require path (e.g. "components" in the case of #2838) isn't relative? So essentially doing what this code does in getFullPath?

While we are modifying this file, I would suggest we also fix the glaring bug that it calls require immediately, rather than later when req is called. This is completely inconsistent with what webpack does.

igor-dv · 2018-01-31T08:02:12Z

would it not make more sense to not call path.resolve() if the root require path (e.g. "components" in the case of #2838) isn't relative? So essentially doing what this code does in getFullPath?

Probably you are right... Will try to test it with a repo from the issue.

While we are modifying this file, I would suggest we also fix the glaring bug that it calls require immediately, rather than later when req is called. This is completely inconsistent with what webpack does.

Why is it a bug? it's a mock of what webpack does, so you can decide how to take care of it =)

tmeasday · 2018-01-31T08:35:29Z

Why is it a bug?

Because the require happens at a different time -- in this code (direct from the docs):

// In storyshots, the require happens here:
const req = require.context('../src/components', true, /\.stories\.js$/)

function loadStories() {
  // in webpack it happens here:
  req.keys().forEach((filename) => req(filename))
}

configure(loadStories, module);

This has caused bugs in the past, for instance if you add a decorator between the two..

tmeasday · 2018-01-31T08:35:49Z

[We don't have to fix this issue in this PR]

igor-dv · 2018-01-31T08:38:26Z

Is there any open bug on it?

tmeasday · 2018-01-31T08:40:39Z

Only in my head for some reason I think..

igor-dv · 2018-01-31T16:00:59Z

@tmeasday, just checked with an absolute path, and path.resolve didn't do anything - left the absolute path as is and everything worked. So I think it probably checks internally for the relativity.

tmeasday · 2018-01-31T23:57:01Z

@igor-dv I created an issue here: #2894. Note that the original issue (#2838) will also have this problem ;)

@tmeasday, just checked with an absolute path, and path.resolve didn't do anything - left the absolute path as is and everything worked. So I think it probably checks internally for the relativity.

Yes, but not so much with "components", it treats this equivalently to "./components". i.e.:

> path.resolve('/a', '/b') # 1.
'/b'
> path.resolve('/a', 'b') # 2.
'/a/b'
> path.resolve('/a', './b') # 3.
'/a/b'

The idea of using path.resolve in this file is so relative paths (#3) are relative to the .storyshots/config.js NOT the cwd. We don't need to touch {whatever you call paths without a slash} (ie #2)

igor-dv · 2018-02-01T08:40:22Z

Aha ! Now I understand what you are talking about 😈 I'll do it

…ing the full path

tmeasday · 2018-02-02T00:42:47Z

addons/storyshots/src/require_context.js

@@ -40,6 +41,14 @@ function isRelativeRequest(request) {
  );
 }

+function getFullPath(dirname, request) {
+  if (isRelativeRequest(request) || !process.env.NODE_PATH) {


I don't think it needs to be this complex; process.env.NODE_PATH will automatically apply if you require something non-relative, won't it?

So it can just be:

return isRelativeRequest(request) ? path.resolve(dirname, request) : dirname;

Checked it now, and it's not applied. It's just resolved according to the cwd. 🤷‍♂️

Oooh, because we need to a fs.readdirSync in the directory. Sorry I am bit slow 😊

Do we not need to worry about other require rules in this case? For instance if I do require.context('foo') and I have node_modules/foo or ../node_modules/foo or .. etc

I guess this is better than nothing though!

Yeah... I would say let's fix on request =)
Also, why would anyone require.context('foo'), when foo is a node module 🤔 what should we expect to happen

tmeasday · 2018-02-02T12:23:37Z

that’s fair..

…

On Fri, 2 Feb 2018 at 9:46 pm, Igor ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In addons/storyshots/src/require_context.js <#2873 (comment)>: > @@ -40,6 +41,14 @@ function isRelativeRequest(request) { ); } +function getFullPath(dirname, request) { + if (isRelativeRequest(request) || !process.env.NODE_PATH) { Yeah... I would say let's fix on request =) Also, why would anyone require.context('foo'), when foo is a node module 🤔 what should we expect to happen — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2873 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIFytc7Wk99UGYMCrSvXQISlmqukZ8Aks5tQueXgaJpZM4RydSt> .

Resolve directory in require.context based on NODE_PATH if provided.

dd0c4ba

igor-dv added feature request compatibility with other tools addon: storyshots labels Jan 30, 2018

igor-dv self-assigned this Jan 30, 2018

Merge branch 'master' into storyshots-support-NODE_PATH

233d73d

igor-dv requested a review from tmeasday January 30, 2018 19:16

Merge branch 'master' into storyshots-support-NODE_PATH

2ca03e4

Merge branch 'master' into storyshots-support-NODE_PATH

fe8d9b8

igor-dv and others added 3 commits February 1, 2018 12:31

Check if the requested directory in context is relative when calculat…

7423f94

…ing the full path

Merge branch 'master' into storyshots-support-NODE_PATH

e422f3a

Merge branch 'master' into storyshots-support-NODE_PATH

4754799

tmeasday requested changes Feb 2, 2018

View reviewed changes

tmeasday approved these changes Feb 2, 2018

View reviewed changes

Merge branch 'master' into storyshots-support-NODE_PATH

5d3e6eb

igor-dv merged commit 968209d into master Feb 2, 2018

igor-dv deleted the storyshots-support-NODE_PATH branch February 2, 2018 12:43

dependencies bot mentioned this pull request Jan 15, 2021

Update 13 dependencies from npm #2555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

env.NODE_PATH support in Storyshots #2873

env.NODE_PATH support in Storyshots #2873

igor-dv commented Jan 30, 2018

codecov bot commented Jan 30, 2018 •

edited

Loading

tmeasday commented Jan 30, 2018 •

edited

Loading

igor-dv commented Jan 31, 2018 •

edited

Loading

tmeasday commented Jan 31, 2018

tmeasday commented Jan 31, 2018

igor-dv commented Jan 31, 2018

tmeasday commented Jan 31, 2018

igor-dv commented Jan 31, 2018 •

edited

Loading

tmeasday commented Jan 31, 2018 •

edited

Loading

igor-dv commented Feb 1, 2018

tmeasday Feb 2, 2018 •

edited

Loading

igor-dv Feb 2, 2018

tmeasday Feb 2, 2018

igor-dv Feb 2, 2018

tmeasday commented Feb 2, 2018 via email

env.NODE_PATH support in Storyshots #2873

env.NODE_PATH support in Storyshots #2873

Conversation

igor-dv commented Jan 30, 2018

What I did

How to test

codecov bot commented Jan 30, 2018 • edited Loading

Codecov Report

tmeasday commented Jan 30, 2018 • edited Loading

igor-dv commented Jan 31, 2018 • edited Loading

tmeasday commented Jan 31, 2018

tmeasday commented Jan 31, 2018

igor-dv commented Jan 31, 2018

tmeasday commented Jan 31, 2018

igor-dv commented Jan 31, 2018 • edited Loading

tmeasday commented Jan 31, 2018 • edited Loading

igor-dv commented Feb 1, 2018

tmeasday Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

igor-dv Feb 2, 2018

Choose a reason for hiding this comment

tmeasday Feb 2, 2018

Choose a reason for hiding this comment

igor-dv Feb 2, 2018

Choose a reason for hiding this comment

tmeasday commented Feb 2, 2018 via email

codecov bot commented Jan 30, 2018 •

edited

Loading

tmeasday commented Jan 30, 2018 •

edited

Loading

igor-dv commented Jan 31, 2018 •

edited

Loading

igor-dv commented Jan 31, 2018 •

edited

Loading

tmeasday commented Jan 31, 2018 •

edited

Loading

tmeasday Feb 2, 2018 •

edited

Loading