microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.55k stars 2.74k forks source link

rush generate: merge-styles build fails due to missing testURL in jest config #5714

Closed Jahnp closed 6 years ago

Jahnp commented 6 years ago

Bug Report

Priorities and help requested:

Are you willing to submit a PR to fix? Yes - I have a local fix Requested priority: Blocking - build fails

Describe the issue:

Attempting to build merge-styles--both locally with npm run build and via rush build -t merge-styles--fails during the jest task.

Here is a sample error returned for each test:

 FAIL  src/keyframes.test.tsun...
  ● Test suite failed to run

    SecurityError: localStorage is not available for opaque origins

      at Window.get localStorage [as localStorage] (../../common/temp/node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
          at Array.forEach (<anonymous>)

This seems to be related to an issue with JSDom via jest. More details here: https://github.com/jsdom/jsdom/issues/2304 .

I was able to confirm that adding 'testURL': 'http://localhost' produces a clean build.

If applicable, please provide a codepen repro:

N/A as this is a build-time issue.

cliffkoh commented 6 years ago

Hi Peter, could you run npm ls > dep.txt in your common\temp folder and share that txt file?

I suspect the root cause is that you have a different dependency tree which is why this issue is not yet more broadly encountered.

Jahnp commented 6 years ago

Thanks Cliff, that's a good idea--that thought had occurred to me but I hadn't thought to check the tree. I'll take a look and share back.

Jahnp commented 6 years ago

Here's a dump: https://1drv.ms/t/s!AmfPnD1fEok_n_x6l1IV6uJYT9qjXA

I'll take a look soon.

cliffkoh commented 6 years ago

Thanks Peter. In your dump, you have jsdom@11.12.0 installed but on my local machine and in the npm_shrinkwrap.json (https://github.com/OfficeDev/office-ui-fabric-react/blob/0b3d2f0b993d82d67c8c42017698891f8a1269ad/common/config/rush/npm-shrinkwrap.json#L19595), we have an earlier version (11.11.0).

This should be the explanation for the difference in build behavior.

What I could do is to try to repro this tomorrow with a fresh clone.

cliffkoh commented 6 years ago

Hi Peter,

I have actually done a fresh clone and am not able to reproduce (which is what I expected otherwise our various CI processes including the daily publish will be failing).

Have you perhaps done a rush generate locally? If so, it would imply that the problem you hit is a real problem that will affect the next unfortunate soul to run that command. Probably, the right fix is to wait for jest to publish a right fix for this problem and consume that fixed version of jest.

Jest has this issue tracked at: https://github.com/facebook/jest/issues/6766

Jahnp commented 6 years ago

Hey Cliff, thank you for digging in! Great detective work. I did indeed run rush generate--I'd had some rush install issues which prompted rush to suggest running generate.

It seems reasonable to wait for a jest fix. Feels like we should keep this open until that's the case--thoughts?

cliffkoh commented 6 years ago

Are you blocked on needing to rush generate?

I have a suggestion for a workaround if so (manually edit npm-shrinkwrap.json to point to the old version of jsdom). I will update the title of this to be clearer.

Jahnp commented 6 years ago

Thanks! Before opening this issue, I was able to unblock myself locally with the existing version of jsdom by adding 'testURL': 'http://localhost' to jest-resources per the discussion here: https://github.com/jsdom/jsdom/issues/2304

This seems to be a pretty common fix for this issue. I'd be willing to open a PR with this fix in the short term in case others hit this issue and also need to run rush generate.

cliffkoh commented 6 years ago

Fixed in #5745.