[Packager] Generate module name seperated by '/' on Windows. #2813
Conversation
tdzl2003
changed the title
facebook-github-bot
added
the
CLA Signed
|
@martinbigio What do you think? |
By using module path as module id cause issues when resolving module dependence,change Win path like'\\' or '\' to '/' before generate bundle code.
|
should you also update the the on the constructor? Take a look also on #2829. I don't have a windows computer to make sure either this or that is sufficient to fix the path issue |
|
#2829 still causing errors on my machine. It changed module define and dependencies, but not changing path in require statement。 I don't think change "path" property is necessary. That may effect to many places. File path on windows can still performed with '', only the module name used in __d and require should replace '' to '/' |
|
The change looks fine to me. I wonder if the mentioned PR (which we've already landed) wouldn't be necessary with this. Could you if we could keep the internals as they used to be (reverting that diff)? |
|
No it's not necessary. #2829 still causing errors on my machine. It changed module define and dependencies, but not changing path in require statement。 |
|
do you mind including the revert of that on this PR? |
|
#2829 merged and reverted. |
| @@ -56,7 +56,7 @@ class Module { | |||
| return this.path; | |||
| } | |||
|
|
|||
| return path.join(name, path.relative(p.root, this.path)); | |||
| return path.join(name, path.relative(p.root, this.path)).replace(/\\/g, '/'); | |||
There was a problem hiding this comment.
Is there any way path.join can return null or undefined?
There was a problem hiding this comment.
not if the parameters are defined
|
Cool! Is this good to go now? |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/413996215466890/int_phab to review. |
ghost
closed this
in
Summary: I think packager on different platform should generate same output if possible. So packager should replace '\\' in module name with '/' on Windows. Closes facebook#2813 Reviewed By: @svcscm Differential Revision: D2458634 Pulled By: @martinbigio
Summary: I think packager on different platform should generate same output if possible. So packager should replace '\\' in module name with '/' on Windows. Closes facebook/react-native#2813 Reviewed By: @svcscm Differential Revision: D2458634 Pulled By: @martinbigio
I think packager on different platform should generate same output if possible. So packager should replace '' in module name with '/' on Windows.