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.

Sign up for GitHub

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

Jump to bottom

Prefer copyFileSync from here over native #1440

Merged
merged 2 commits into from
Sep 14, 2017
Merged

Prefer copyFileSync from here over native #1440

merged 2 commits into from
Sep 14, 2017

Conversation

ahocevar
Copy link
Contributor

@ahocevar ahocevar commented Sep 13, 2017 •
edited
Loading

lib/jsdoc/fs.js always prefers native implementations over its overrides. With node v8.5 implementing copyFile and copyFileSync natively with a different API, building docs broke.

I've made it so overrides can now easily be preferred by adding them to the alwaysOverride object.

Fixes #1438.

@ahocevar ahocevar mentioned this pull request Sep 13, 2017
Closed
@ahocevar ahocevar force-pushed the patch-1 branch 2 times, most recently from b12f5e6 to 30955ee Compare September 13, 2017 09:52
@ahocevar
Copy link
Contributor Author

Tests pass for me locally with node v4, v6 and v8. Not sure what's wrong in Travis CI.

@ahocevar
Copy link
Contributor Author

babylon is the offender. JSDoc does not work with 7.0.0-beta.22, but it does work with 7.0.0-beta.19. I've changed package.json accordingly.

tschaub
tschaub approved these changes Sep 13, 2017
View reviewed changes
Copy link
Contributor

@tschaub tschaub left a comment

Choose a reason for hiding this comment

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

Looks like a good fix.

@hegemonic hegemonic merged commit 932c357 into jsdoc:master Sep 14, 2017
@hegemonic
Copy link
Contributor

Thanks for the fix! I'll release this as JSDoc 3.5.5 shortly.

@hegemonic
Copy link
Contributor

(Also, not sure what's up with Babylon; I'll investigate that separately.)

hegemonic pushed a commit that referenced this pull request Sep 14, 2017
@ahocevar @hegemonic
Prefer copyFileSync from here over native (#1440)
01192fa
@greggman
Copy link

greggman commented Sep 14, 2017 •
edited
Loading

Just throwing this out there, I probably haven't thought it all the way through but ...

Why not just always override all functions? It seems like this method of implementing some functions but then replacing them with fs ones is destined to have more issues like this in the future since there's no control or idea of what the people upstream will decide the semantics of the functions they make are.

In other words, move the part that re-exports all the fs functions to the top of the file, then add the jsdoc versions over them. Don't check if there is already a native version.

@arwhyte arwhyte mentioned this pull request Oct 2, 2017
Closed
lheberlie added a commit to bsvensson/jsdoc that referenced this pull request Oct 11, 2017
@lheberlie
Merge tag 3.5.5 into esrijs-sdk-3.5.5
05f540e 
* commit '4c7f9049a0e8c6d014e4d7dd1335de7314955493':
  3.5.5
  3.5.5 changelog
  Prefer copyFileSync from here over native (jsdoc#1440)
  start 3.5.5-dev

# Conflicts:
#	package.json
@ialarmedalien ialarmedalien mentioned this pull request Nov 3, 2017
Closed
@ahocevar ahocevar deleted the patch-1 branch December 20, 2017 22:01
@cwalther cwalther mentioned this pull request Jun 25, 2018
Closed
lheberlie added a commit to bsvensson/jsdoc that referenced this pull request May 2, 2019
@lheberlie
Merge branch 'master' into esrijs-sdk-3.6.0
90aacf5 
* master: (95 commits)
  update dependencies and supported Node.js versions
  3.5.5 changelog
  Prefer copyFileSync from here over native (jsdoc#1440)
  upgrade Babylon
  fix test breakage
  3.5.4 changelog
  prevent crash when an anonymous class is passed as a parameter (jsdoc#1416)
  hide the signature in the heading for classes with hidden constructors (jsdoc#1397)
  chore(package): update nyc to version 11.1.0 (jsdoc#1417)
  add `templates.useShortNamesInLinks` config option (jsdoc#738)
  allow users to specify a highlighter for Markdown code blocks (jsdoc#1412)
  document `longnamesToTree`, plus other doc improvements (jsdoc#43)
  move namespaces and interfaces up in the nav (jsdoc#1410)
  don't pretty-print code blocks that begin with "```plain" (jsdoc#1361)
  improve comment
  make the `exports` tag work correctly when combined with the `enum` tag (jsdoc#970)
  fix Node.js 4.x
  update tested Node.js versions
  use the markdown-it Markdown parser by default (jsdoc#1243)
  enable more Babylon options (jsdoc#1411)
  ...

# Conflicts:
#	.travis.yml
#	README.md
#	lib/jsdoc/src/astbuilder.js
#	lib/jsdoc/src/visitor.js
#	lib/jsdoc/util/markdown.js
#	lib/jsdoc/util/templateHelper.js
#	package.json
lheberlie added a commit to bsvensson/jsdoc that referenced this pull request May 6, 2019
@lheberlie
Merge branch 'releases/3.6' into esrijs-sdk-3.6.1
92ba6c3 
* releases/3.6: (109 commits)
  Add 3.6.1 changelog.
  3.6.1
  Parse type applications correctly in Node.js 12. (jsdoc#1643)
  Update .gitignore.
  3.6.0
  Add 3.6.0 changelog.
  Update dependencies, plus the URLs for the GitHub repos and docs.
  update docs with new template (jsdoc#1604)
  switch to new-ish ECMAScript syntax
  update ESLint config
  migrate from `babylon` to `@babel/parser`
  Update ajv to the latest version 🚀 (jsdoc#1599)
  only run CI with Node.js versions that actually exist
  migrate from `markdown-it-named-headers` to `markdown-it-anchor` (jsdoc#1481)
  update dependencies and supported Node.js versions
  3.5.5 changelog
  Prefer copyFileSync from here over native (jsdoc#1440)
  upgrade Babylon
  fix test breakage
  3.5.4 changelog
  ...

# Conflicts:
#	.gitignore
#	.travis.yml
#	CHANGES.md
#	LICENSE.md
#	README.md
#	cli.js
#	lib/jsdoc/fs.js
#	lib/jsdoc/path.js
#	lib/jsdoc/src/astbuilder.js
#	lib/jsdoc/src/handlers.js
#	lib/jsdoc/src/parser.js
#	lib/jsdoc/src/visitor.js
#	lib/jsdoc/src/walker.js
#	lib/jsdoc/util/logger.js
#	lib/jsdoc/util/markdown.js
#	lib/jsdoc/util/templateHelper.js
#	package.json
#	test/specs/documentation/alias.js
#	test/specs/documentation/anonymousclassparam.js
#	test/specs/documentation/arrowfunction.js
#	test/specs/documentation/classproperties.js
#	test/specs/documentation/this.js
#	test/specs/documentation/trailingcomment.js
#	test/specs/jsdoc/name.js
#	test/specs/jsdoc/path.js
#	test/specs/jsdoc/src/astnode.js
#	test/specs/jsdoc/src/visitor.js
lheberlie added a commit to bsvensson/jsdoc that referenced this pull request May 7, 2019
@lheberlie
Merge branch 'releases/3.6' into esrijs-sdk-3.6.1
6d59663 
* releases/3.6: (109 commits)
  Add 3.6.1 changelog.
  3.6.1
  Parse type applications correctly in Node.js 12. (jsdoc#1643)
  Update .gitignore.
  3.6.0
  Add 3.6.0 changelog.
  Update dependencies, plus the URLs for the GitHub repos and docs.
  update docs with new template (jsdoc#1604)
  switch to new-ish ECMAScript syntax
  update ESLint config
  migrate from `babylon` to `@babel/parser`
  Update ajv to the latest version 🚀 (jsdoc#1599)
  only run CI with Node.js versions that actually exist
  migrate from `markdown-it-named-headers` to `markdown-it-anchor` (jsdoc#1481)
  update dependencies and supported Node.js versions
  3.5.5 changelog
  Prefer copyFileSync from here over native (jsdoc#1440)
  upgrade Babylon
  fix test breakage
  3.5.4 changelog
  ...

# Conflicts:
#	.gitignore
#	.travis.yml
#	CHANGES.md
#	LICENSE.md
#	README.md
#	cli.js
#	lib/jsdoc/fs.js
#	lib/jsdoc/path.js
#	lib/jsdoc/src/astbuilder.js
#	lib/jsdoc/src/handlers.js
#	lib/jsdoc/src/parser.js
#	lib/jsdoc/src/visitor.js
#	lib/jsdoc/src/walker.js
#	lib/jsdoc/util/logger.js
#	lib/jsdoc/util/markdown.js
#	lib/jsdoc/util/templateHelper.js
#	package.json
#	test/specs/documentation/alias.js
#	test/specs/documentation/anonymousclassparam.js
#	test/specs/documentation/arrowfunction.js
#	test/specs/documentation/classproperties.js
#	test/specs/documentation/this.js
#	test/specs/documentation/trailingcomment.js
#	test/specs/jsdoc/name.js
#	test/specs/jsdoc/path.js
#	test/specs/jsdoc/src/astnode.js
#	test/specs/jsdoc/src/visitor.js
@thw0rted thw0rted mentioned this pull request Feb 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tschaub tschaub tschaub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ahocevar @hegemonic @greggman @tschaub