Fix aliasing with path.resolve() issue on windows #22

mhhegazy · 2017-05-15T06:34:19Z

@Rich-Harris Could you take a look?

Added 3 test cases:

Platform path.resolve('file-without-extension') aliasing
Windows absolute path aliasing
Platform path.resolve('file-with.ext') aliasing

This fix:

Build Error: not found the fbjs\lib\warning facebook/react#9350
Can't contribute to React on Windows facebook/react#9540 [only the part that related to rollup-plugin-alias]
Unable to build Vue on Windows using npm/yarn run build vuejs/vue#4338

And Closes #11

added 3 test cases: - alias with path.resolve() with file without extention - alias with windows absolute path - alias with path.resolve() with file with extention

passes test cases: - Platform path.resolve('file-without-extension') aliasing - Windows absolute path aliasing - Platform path.resolve('file-with.ext') aliasing

frostney · 2017-05-22T14:10:47Z

test/index.js

@@ -1,10 +1,11 @@
 import test from 'ava';
-import { posix as path } from 'path';
+import paltformPath, { posix as path } from 'path';


Shouldn't this be platformPath?

frostney · 2017-05-22T14:12:52Z

I'm not sure platformPath is a good export name here to be honest. I would rather prefer import path, { posix } from 'path' and change the path references to posix.

Let know what you think @mhhegazy

mhhegazy · 2017-05-23T01:58:54Z

@frostney I agree this will be more clear

make it more clear

mhhegazy · 2017-09-17T03:57:56Z

@frostney please rerun checks again; it was broken because of resolved issue on appveyor.

we should resolve this issue as soon as possible because it break build of many projects on windows

ovirovkin · 2017-10-17T21:46:02Z

@mhhegazy Is there any reason why this fix cannot be pushed?

mhhegazy · 2017-10-18T20:09:38Z

@ovirovkin do mean merged? I don't know!

I have tested it in cases that used in react and vue and it worked as desired and this only pull request that resolve this problem.

mhhegazy · 2017-10-18T20:22:16Z

@ovirovkin
what issue that bring you here?

is it react build on windows or other issue?

if react please check Fix build on window facebook/react#10982
if other please tell me more about your case

ovirovkin · 2017-10-18T20:43:52Z

@mhhegazy Yes, it's react build on windows issue. I fixed build as you suggested here: #9540 but it's causing 23 failed tests. So I was wondered can this be merged, to fix an issue properly. I don't know why they are not failing for you, also I have only 1465 tests total and you have 2559 on a screenshots.

mhhegazy · 2017-10-18T22:35:13Z

you can try to clone PR repo (https://github.com/mhhegazy/react.git) and build it and tell me what is the result of build and test.
which tests are failing?

Rich-Harris · 2017-10-19T02:10:14Z

thanks all, LGTM. Will cut a new release soon

ovirovkin · 2017-10-19T22:00:31Z

@mhhegazy Thanks for your help, your branch looks better. But there is still 2 suits are failing. Results:

Summary of all failing tests
 FAIL  src\renderers\dom\shared\__tests__\ReactDOMServerIntegration-test.js
  ● Test suite failed to run

    C:\Users\User\Documents\react-test\react\src\renderers\dom\shared\__tests__\ReactDOMServerIntegration-test.js:41
    async function expectErrors(fn, count) {
          ^^^^^^^^

    SyntaxError: Unexpected token function

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:292:17)
      at undefined.next (native)
      at handle (node_modules/worker-farm/lib/child/index.js:44:8)
      at process.<anonymous> (node_modules/worker-farm/lib/child/index.js:51:3)
      at emitTwo (events.js:106:13)
      at process.emit (events.js:191:7)
      at process.nextTick (internal/child_process.js:787:12)
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)


 FAIL  src\renderers\__tests__\ReactComponent-test.js
  ● Test suite failed to run

    C:\Users\User\Documents\react-test\react\src\renderers\__tests__\ReactComponent-test.js:429
      it('throws if a plain object is used as a child when using SSR', async function () {
                                                                       ^^^^^

    SyntaxError: missing ) after argument list

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:292:17)
      at undefined.next (native)
      at handle (node_modules/worker-farm/lib/child/index.js:44:8)
      at process.<anonymous> (node_modules/worker-farm/lib/child/index.js:51:3)
      at emitTwo (events.js:106:13)
      at process.emit (events.js:191:7)
      at process.nextTick (internal/child_process.js:787:12)
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)

mhhegazy · 2017-10-20T00:34:24Z

I will take a look at it,
this not related to the build or rollup-plugin-alias so let complete conversation on facebook/react#9540

ovirovkin · 2017-10-20T18:53:32Z

@mhhegazy Thank you, I really appreciate your help.

mhhegazy · 2017-10-21T21:14:57Z

@ovirovkin you are welcome, I'll get into testing in next weekend,
but for now could you approve that build completed successfully here facebook/react#10982

mhhegazy added 2 commits May 15, 2017 08:00

Add test cases for aliasing with path.resolve()

638f400

added 3 test cases: - alias with path.resolve() with file without extention - alias with windows absolute path - alias with path.resolve() with file with extention

Fix aliasing with path.resolve() issue on windows

f752133

passes test cases: - Platform path.resolve('file-without-extension') aliasing - Windows absolute path aliasing - Platform path.resolve('file-with.ext') aliasing

This was referenced May 15, 2017

Can't contribute to React on Windows facebook/react#9540

Closed

"Could not load F:\tinybear\tinybearLab\rollup\libs/m1" in windows #11

Closed

frostney reviewed May 22, 2017

View reviewed changes

frostney mentioned this pull request May 22, 2017

fix #11 support window path #12

Closed

mhhegazy added 2 commits May 23, 2017 04:05

update import names for path and poisx

e423fe6

make it more clear

Merge branch 'master' into fix-windows-resolved-alias

dc5556a

add comment to test

8a194dc

Merge branch 'master' into fix-windows-resolved-alias

2bec324

Rich-Harris merged commit c7770dd into rollup:master Oct 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix aliasing with path.resolve() issue on windows #22

Fix aliasing with path.resolve() issue on windows #22

mhhegazy commented May 15, 2017 •

edited

Loading

frostney May 22, 2017

frostney commented May 22, 2017 •

edited

Loading

mhhegazy commented May 23, 2017

mhhegazy commented Sep 17, 2017

ovirovkin commented Oct 17, 2017

mhhegazy commented Oct 18, 2017 •

edited

Loading

mhhegazy commented Oct 18, 2017

ovirovkin commented Oct 18, 2017 •

edited

Loading

mhhegazy commented Oct 18, 2017 •

edited

Loading

Rich-Harris commented Oct 19, 2017

ovirovkin commented Oct 19, 2017

mhhegazy commented Oct 20, 2017

ovirovkin commented Oct 20, 2017

mhhegazy commented Oct 21, 2017

Fix aliasing with path.resolve() issue on windows #22

Fix aliasing with path.resolve() issue on windows #22

Conversation

mhhegazy commented May 15, 2017 • edited Loading

Added 3 test cases:

This fix:

And Closes #11

frostney May 22, 2017

Choose a reason for hiding this comment

frostney commented May 22, 2017 • edited Loading

mhhegazy commented May 23, 2017

mhhegazy commented Sep 17, 2017

ovirovkin commented Oct 17, 2017

mhhegazy commented Oct 18, 2017 • edited Loading

mhhegazy commented Oct 18, 2017

ovirovkin commented Oct 18, 2017 • edited Loading

mhhegazy commented Oct 18, 2017 • edited Loading

Rich-Harris commented Oct 19, 2017

ovirovkin commented Oct 19, 2017

mhhegazy commented Oct 20, 2017

ovirovkin commented Oct 20, 2017

mhhegazy commented Oct 21, 2017

mhhegazy commented May 15, 2017 •

edited

Loading

frostney commented May 22, 2017 •

edited

Loading

mhhegazy commented Oct 18, 2017 •

edited

Loading

ovirovkin commented Oct 18, 2017 •

edited

Loading

mhhegazy commented Oct 18, 2017 •

edited

Loading