Skip to content

Conversation

@JoelEinbinder
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't path.basename(curSource) === file?

Copy link
Contributor

Choose a reason for hiding this comment

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

root is not used

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do the path-joining magic in the very beginning so that it's easier to follow what's going on.

const from = path.join(source, file);
const to = path.join(target, file);
...

Copy link
Contributor

Choose a reason for hiding this comment

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

let's check for file explicitly. It would not be nice to write over a symlink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what you mean by this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean:

 if (fs.lstatSync(curSource).isDirectory()) {
  ...
} else if (fs.lstatSync(curSource).isFile()) {
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't copying everything into a node6/ work? this would spare you from this replace.

I.e.:

removeSync('node6');
copyFolder('lib', 'node6/lib');
copyFolder('test', 'node6/test');
copyFile('index.js', 'node6/index.js');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets really tricky because we load some utils from lib, and some utils are hard to convert. There are also paths for things like package.json and .local-chromium which get confusing.

@aslushnikov aslushnikov merged commit cd81944 into puppeteer:master Aug 26, 2017
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.

2 participants