Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

path.spit used with platform specific separator #2

Closed
wants to merge 6 commits into from

Conversation

deepak1556
Copy link

added minor changes to run successful test in windows. There are still some files left out with unsuccessful test . Sent a PR to start conversation on the tests.

@deepak1556
Copy link
Author

@jordwalke it is possible using path.sep property http://nodejs.org/api/path.html#path_path_sep and yeah i have signed the CLA.

@deepak1556
Copy link
Author

@jordwalke all unit test now pass, can you check them out.

@zpao
Copy link

zpao commented Aug 28, 2013

I have a fix for the docblock parsing in https://github.com/facebook/react/pull/230/files which we should probably take here - it should have a little bit better coverage. I need to also get that into jstransform (we have the docblock file duplicated so we should try to keep them consistent as much as possible)

@deepak1556
Copy link
Author

+1 for docblock parsing.

}
return [dirname];
return [path.join(dirname)];
Copy link

Choose a reason for hiding this comment

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

this doesn't seem necessary

Copy link
Author

Choose a reason for hiding this comment

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

When called without any haste roots by IndexConfiguration method from ConfigurationTrie.js the dirname returned will conflict with var parts = config_path.split(path.sep); , the first test of ProjectConfiguration-test.js will not pass.

@jordwalke
Copy link

We tried patching this into react-page's node_modules folder (deep), and it didn't seem to find files correctly - granted it didn't do it correctly before either - but I'm very excited about getting solid windows support so your help is appreciated. Thoughts on what's going wrong with react-page+this patch?
www.github.com/facebook/react-page

@deepak1556
Copy link
Author

In HasteDependencyLoader.js the id is null in all the resources object returned by loadOrderedDependencies, but the paths are returned perfectly. Commenting the condition in getOrderedDependencies
if (!resource || !resource.id || orderedResources[resource.id]) { return; }
the execution continues but an error in docblock parsing of src/index.js is thrown..

C:\github\react-page\node_modules\react-tools\node_modules\jstransform\src\docbl
ock.js:24
  var match = contents.match(docblockRe);
                       ^
TypeError: Object /**
 * Copyright 2013 Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 * @jsx React.DOM
 */

var Banner = require('./elements/Banner/Banner.js');
var React = require('React');
var SiteBoilerPlate = require('./core/SiteBoilerPlate.js');
var VectorWidget = require('./elements/VectorWidget/VectorWidget.js');

var index = React.createClass({
  render: function() {
    return (
      <SiteBoilerPlate>
        <Banner bannerMessage="Welcome to React"/>
        <VectorWidget />
      </SiteBoilerPlate>
    );
  }
});

module.exports = index;
 has no method 'match'
    at Object.extract (C:\github\react-page\node_modules\react-tools\node_module
s\jstransform\src\docblock.js:24:24)
    at getDocblock (C:\github\react-page\node_modules\react-tools\node_modules\j
stransform\src\utils.js:351:39)
    at Function.test (C:\github\react-page\node_modules\react-tools\vendor\fbtra
nsform\transforms\react.js:182:13)
    at walker (C:\github\react-page\node_modules\react-page-middleware\node_modu
les\jstransform\src\jstransform.js:201:21)
    at analyzeAndTraverse (C:\github\react-page\node_modules\react-page-middlewa
re\node_modules\jstransform\src\utils.js:392:9)
    at traverse (C:\github\react-page\node_modules\react-page-middleware\node_mo
dules\jstransform\src\jstransform.js:146:3)
    at transform (C:\github\react-page\node_modules\react-page-middleware\node_m
odules\jstransform\src\jstransform.js:236:3)
    at C:\github\react-page\node_modules\react-page-middleware\src\Packager.js:2
09:25
    at C:\github\react-page\node_modules\react-page-middleware\src\Packager.js:2
35:29
    at [object Object].<anonymous> (fs.js:115:5)

all other files are parsed without any errors except for the react-page src files. Also could you add platform-specific regex in JSMockLoader.js and JSTestLoader.js

unction JSMockLoader(options) {
  this.options = options = options || {};
  this.pathRe = options.matchSubDirs ?
    /(?:\\|^)__mocks__\\(.+)\.js$/ :
    /(?:\\|^)__mocks__\\([^\\]+)\.js$/;
}

@jordwalke
Copy link

Yeah - the ids are important. I saw the same thing (ids not being extracted). I suspect the issue could be in ProjectConfiguration.js, in the method: ProjectConfiguration.prototype.resolveID. I notice you used path.sep which is what I would have suggested (and thought would work) but it still doesn't seem to be resolving the ids there. Do you also need to use path.join instead of concating?

Feel free to create a separate issue for the mock stuff. As soon as we nail this main issue, we can tackle the mock loader next.

@deepak1556
Copy link
Author

yeah path.join takes of concatenating separators for different platforms, but i dnt think the problem is in ProjectConfiguration.prototype.resolveID method because its never being called in my case. On commenting the line record.newResource = resource; in the analyeChanged callback of run method in MapUpdateTask.js i got all resources with ids but they were same as the paths not parsed to oly the file name and the got this error

500 Error: Following module not in specified search paths: C:\github\react-page\src\index.js

do you know what might me the problem here.?

@jordwalke
Copy link

ProjectConfiguration.prototype.resolveID is certainly getting called for me on Mac OS X, so I believe we should continue tracking down why that's never getting called.

I believe it should be getting called from JSResource.js on the line that says js.id = configuration.resolveID(js.path); - notice the if(configuration) check and the !js.id check. Can you tell me why it's failing there for a file like index.js that definitely should have configuration and a !js.id?

BTW: configuration is the project configuration object that represents a package.json project (index.js would have a configuration for the react-page project because there is a package.json file)

@zpao
Copy link

zpao commented Jul 3, 2014

I think this is mostly fixed up, so I'm going to close out. Let me know if there are in fact places that still need to be fixed.

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

Successfully merging this pull request may close these issues.

4 participants