Ace version fix #226

danielweck · 2019-04-29T09:36:08Z

Fixes #198

package.json from @daisy/ace-config used as authoritative source of truth for global Ace version, used in CLI(s) (via meow), verbose logger (via winston), and JSON/HTML report (via @daisy/ace-report).

…ruth for global Ace version, used in CLI(s) (meow), verbose logger (winston), and JSON/HTML report.

danielweck · 2019-04-29T09:37:02Z

PS: this bumps Ace to version 1.0.3 :)

… see Snyk warnings in PRs anyway)

danielweck · 2019-04-29T10:15:46Z

Note that I disabled Snyk Travis checks, which used to break the PR builds unnecessarily. See:
https://travis-ci.org/daisy/ace/pull_requests

The PR "homepage" shows the Snyk warnings anyway, so the project lead can check this info before deciding to merge.

rdeltour

I'm wondering if it wouldn't make more sense to use ace-core as the authoritative source of truth for the version. ace-config is a set of utilities which may be quite stable in time (i.e. may not need to be artificially updated for each release), whereas ace-core really represents the core functionality. WDYT?

CHANGELOG.md

packages/ace-cli/package.json

packages/ace-report/package.json

website/config.toml

danielweck · 2019-04-29T16:38:29Z

I'm wondering if it wouldn't make more sense to use ace-core as the authoritative source of truth for the version. ace-config is a set of utilities which may be quite stable in time (i.e. may not need to be artificially updated for each release), whereas ace-core really represents the core functionality. WDYT?

I was concerned about cyclic dependencies. This PR:

@daisy/ace-config ~~~~~~~~~~~~~~~~~
- (no @daisy/* deps)
@daisy/ace-core
- @daisy/ace-report-axe
- @daisy/puppeteer-utils
- @daisy/ace-report
- @daisy/ace-config <<------------------------
- @daisy/epub-utils
- @daisy/jest-puppeteer
@daisy/ace-cli
- @daisy/ace-logger
- @daisy/ace-core
- @daisy/ace-config <<------------------------
@daisy/ace-http
- @daisy/ace-core
- @daisy/ace-logger
- @daisy/ace-config <<------------------------
@daisy/ace-logger
- @daisy/ace-config <<------------------------
@daisy/ace-report-axe
- @daisy/ace-report
@daisy/ace-report
- @daisy/ace-config <<------------------------
@daisy/ace
- @daisy/ace-http
- @daisy/ace-cli
- @daisy/ace-core
@daisy/jest-puppeteer
- @daisy/puppeteer-utils

...vs. if we switch to ace-core's package.json to carry the "global" Ace version:

@daisy/ace-config
- (no @daisy/* deps)
@daisy/ace-core ~~~~~~~~~~~~~~~~~
- @daisy/ace-report-axe !!========================
- @daisy/puppeteer-utils
- @daisy/ace-report !!========================
- @daisy/epub-utils
- @daisy/jest-puppeteer
@daisy/ace-cli
- @daisy/ace-logger
- @daisy/ace-core <<------------------------
- @daisy/ace-config
@daisy/ace-http
- @daisy/ace-core <<------------------------
- @daisy/ace-logger
@daisy/ace-logger
- @daisy/ace-config
@daisy/ace-report-axe
- @daisy/ace-report
@daisy/ace-report
- @daisy/ace-core <<------------------------
- @daisy/ace-config
@daisy/ace
- @daisy/ace-http
- @daisy/ace-cli
- @daisy/ace-core <<------------------------
@daisy/jest-puppeteer
- @daisy/puppeteer-utils

As you can see:

@daisy/ace-core => @daisy/ace-report-axe => @daisy/ace-core CYCLIC
@daisy/ace-core => @daisy/ace-report-axe => @daisy/ace-report => @daisy/ace-core CYCLIC

Although in many cases this doesn't cause the NodeJS interpreter to fail, further down the compiler chain (e.g. WebPack etc. bundlers) this can cause headaches.

What do you think?

…ven monorepo with single root node_modules containing flat deps packages, and single root yarn.lock ... package-lock.json would possibly catch these discrepancies in each of the monorepo's individual packages? ... depends on the build process, really)

rdeltour · 2019-04-29T18:20:36Z

I was concerned about cyclic dependencies

Right, this could be an issue indeed. Just thinking out loud: what about creating a package that would do just this? (e.g.ace-version or ace-meta?). Given that adding package is pretty lightweight with the monorepo setup… That way we don't overload ace-config, and remove the cyclic deps issue.

…l Ace version

danielweck · 2019-04-30T08:46:42Z

A new package ace-meta now provides version info via its package.json (authoritative source of truth for global Ace version). I cleaned-up the PR, reducing it to what is strictly necessary for this bug fix. Could you please review?

Tested with:

node packages/ace[-cli]/bin/ace --version (shows 1.0.3)
node packages/ace[-cli]/bin/ace --verbose --force --outdir PATH_TO_DEST_DIR PATH_TO_SRC_EPUB (shows 1.0.3 in verbose console log, and in generated JSON/HTML report)

rdeltour

Looks good to me!

I just have a question about the need to explicitly declare transitive dependencies, but this is not critical to this PR.

Also, do not forget to squash and update the commit message when merging :-)

rdeltour · 2019-04-30T08:53:56Z

packages/ace-cli/package.json

@@ -18,8 +18,10 @@
  "main": "lib/index.js",
  "bin": "bin/ace.js",
  "dependencies": {
+    "@daisy/ace-config": "^1.0.0",


Do we need an explicit dependency to ace-config, since we're transitively depending on it via ace-core?

Yes, a package should explicitly declare all its direct dependencies.

danielweck · 2019-04-30T09:30:11Z

Is this a suitable commit message?

fix: correct global Ace version.

A consistent Ace version is now provided via the command line, in the verbose logger, and into the generated JSON/HTML reports. The new ace-meta package is the authoritative source of truth for versioning information.

PR closes #198

rdeltour · 2019-04-30T09:40:43Z

looks good!

package.json from @daisy/ace-config used as authoritative source of t…

817b5dc

…ruth for global Ace version, used in CLI(s) (meow), verbose logger (winston), and JSON/HTML report.

danielweck requested review from rdeltour and marisademeglio April 29, 2019 09:36

danielweck mentioned this pull request Apr 29, 2019

Ace version different from command line to HTML report #198

Closed

danielweck added 2 commits April 29, 2019 11:07

version bump in docs too (changelog, website)

fe6c77b

commented-out Travis Snyk integration, breaks build right now (we can…

bd75c96

… see Snyk warnings in PRs anyway)

rdeltour reviewed Apr 29, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

packages/ace-cli/package.json Show resolved Hide resolved

packages/ace-report/package.json Show resolved Hide resolved

website/config.toml Outdated Show resolved Hide resolved

danielweck added 3 commits April 29, 2019 16:54

CHANGELOG is auto-updated, reverting change (see #226 (comment) )

c54ef03

version bumps where needed (see #226 (comment) and #226 (comment) )

34936f7

reverting website version bumps (see #226 (comment) )

1413f46

danielweck added 5 commits April 30, 2019 09:27

using new ace-meta package as authoritative source of truth for globa…

4c39e68

…l Ace version

fixed version mismatch

183b27d

revert ace-config to v1.0.0

1277148

removed unnecessary code change (simplifies diff)

becdfef

version bump for ace-report-axe (had incorrect package deps, now fixed)

264d3c0

danielweck requested a review from rdeltour April 30, 2019 08:48

rdeltour approved these changes Apr 30, 2019

View reviewed changes

danielweck merged commit 822469f into master Apr 30, 2019

danielweck deleted the fix/versions branch April 30, 2019 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ace version fix #226

Ace version fix #226

danielweck commented Apr 29, 2019

danielweck commented Apr 29, 2019

danielweck commented Apr 29, 2019

rdeltour left a comment

danielweck commented Apr 29, 2019

rdeltour commented Apr 29, 2019

danielweck commented Apr 30, 2019

rdeltour left a comment

rdeltour Apr 30, 2019

danielweck Apr 30, 2019

danielweck commented Apr 30, 2019

rdeltour commented Apr 30, 2019

Ace version fix #226

Ace version fix #226

Conversation

danielweck commented Apr 29, 2019

danielweck commented Apr 29, 2019

danielweck commented Apr 29, 2019

rdeltour left a comment

Choose a reason for hiding this comment

danielweck commented Apr 29, 2019

rdeltour commented Apr 29, 2019

danielweck commented Apr 30, 2019

rdeltour left a comment

Choose a reason for hiding this comment

rdeltour Apr 30, 2019

Choose a reason for hiding this comment

danielweck Apr 30, 2019

Choose a reason for hiding this comment

danielweck commented Apr 30, 2019

rdeltour commented Apr 30, 2019