Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix windows paths #52

Merged
merged 1 commit into from
Dec 6, 2017
Merged

Fix windows paths #52

merged 1 commit into from
Dec 6, 2017

Conversation

Yeti-or
Copy link
Member

@Yeti-or Yeti-or commented Dec 4, 2017

closes #51

@Yeti-or Yeti-or requested a review from veged December 4, 2017 13:04
@awinogradov awinogradov self-requested a review December 4, 2017 21:06
Copy link
Member

@awinogradov awinogradov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not winPath, but smth abstract. Like: 'universalPath'

Copy link
Member

@veged veged left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange but ok

@@ -82,7 +83,7 @@ module.exports = function(source) {
cell : bemCell,
exist,
// prepare path for require cause relative returns us string that we couldn't require
path : requiredPath(path.relative(path.dirname(this.resourcePath), entityPath))
path : winPath(requiredPath(path.relative(path.dirname(this.resourcePath), entityPath)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like It's a requiredPath trouble, I told you path for require should be with slash (not a backslash) even on windows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's not a problem with requiredPath, I checked
Take a look at #51
there are no slashes here ./common.blocksAppRouterAppRouter.js
it should be ./common.blocks\AppRouter\AppRouter.js there is nothing wrong with requiredPath . It's problem elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, these guys don't use required-path but unify path too. why?
https://github.com/ant-design/babel-plugin-import/blob/master/src/Plugin.js#L14

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zxqfox
I described all solutions in detals at #26 (comment)

So seems @Yeti-or decided to go with make replace on output of required-path call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, these guys don't use required-path but unify path too. why?

Because they are decided to stick to / instead of escaping \ characters.
The problem is path writing to file where it will be evaluated from './App\App.js' to './AppApp.js' as @Guria described in their comment.

These guys also has a special option customName that can return undetermined result (we can't sure the result won't have \ character) so they just replaces all \ with / that they don't need to escape.

So it's viable solution. But if we'll get some troubles with that in future we should keep that problem in mind.
I'd say we should left a comment in code to this thread at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module not found: Can't resolve './common.blocksAppRouterAppRouter.js' on windows
5 participants