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

Rework package structure #15

Open
3 tasks
parzhitsky opened this issue Jul 20, 2023 · 1 comment
Open
3 tasks

Rework package structure #15

parzhitsky opened this issue Jul 20, 2023 · 1 comment
Labels
Domain: main [Issue / PR] describes change in the functionality, its optimization Domain: meta [Issue / PR] describes change in the development process, documentation, maintenance etc. Pending: unclear [Issue] not yet fully defined Priority: low [Issue / PR] could be addressed at any convenient time Type: improvement [Issue / PR] addresses lack of a functionality or an open possibility of enhancement Type: investigation [Issue / PR] addresses the need of gaining intel

Comments

@parzhitsky
Copy link
Member

parzhitsky commented Jul 20, 2023

Files that are available to third-party packages

As of v20230101.1.10

✅ – the file is at its place, it's good to go
🔄 – the file might be relocated, which will likely not be an issue
⛔️ – the file should be relocated, which will likely be an issue
💬 – to be decided

Package info files

  • @iso4217/json/LICENSE
  • @iso4217/json/package.json
  • @iso4217/json/README.md

Package meta scripts

  • @iso4217/json/postinstall.js

Business logic

  • ISO 4217 data
    • 💬 @iso4217/json/data.json
    • 💬 @iso4217/json/data-grouped-by-*.json
  • Public scripts
    • 💬 @iso4217/json/build-grouped-by-data-files.{d.ts,d.ts.map,js,js.map}
    • 💬 @iso4217/json/index.{d.ts,d.ts.map,js,js.map}
    • 💬 @iso4217/json/js-xml.type.{d.ts,d.ts.map}
  • Private scripts
    • 🔄 @iso4217/json/write-to-file.{d.ts,d.ts.map,js,js.map}

Usage scenarios of @iso4217/json that involve working with files inside the package

✅ – the scenario is not affected / does not pose any issues
🔄 – the scenario poses fixable issues
⛔️ – the scenario poses non-fixable / hard to fix issues
💬 – to be decided

👍 – a promising argument
👎 – a "not going to be easy" argument

⛔️ Import ISO 4217 data by reading the file

import fs from 'fs'

const file = require.resolve('@iso4217/json/data.json')
const json = fs.readFileSync(file)
const data = JSON.parse(json)
  • 👎 Explicitly depends on ./data.json
    • Not quite.
      • 👎 The file's location in node_modules is explicitly mentioned in README: path/to/node_modules/@iso4217/json/data.json

      • 👍 Although the file must be referenced directly, usage of require.resolve(…) is suggested, which is very different from something like path.resolve(…) and allows for flexibility in file location

        path.resolve('@iso4217/json/data')
        // <cwd>/@iso4217/json/data
        
        require.resolve('@iso4217/json/data')
        // path/to/node_modules/@iso4217/json/data.json
  • 👍 It is not the primary way of importing data
    • 👎 although one that's claimed to be supported
  • Investigate if it is possible to use a symlink (./data.json./dist/data.json)
    • 👎 If symlink is not possible, either the data.json file would have to be copied to src / dist (having two copies of it), or the business logic files would have to refer to the file in the parent folder
      • 👍 Actually, this is how package.json is referenced in many packages, so this seems promising

⛔️ Generating data-grouped-by-*.json files automatically

Upon npm install, unless --ignore-scripts is used, the environment variables ISO4217_JSON_BUILD_DATA_GROUPED_BY_{criteria} specify wither to generate a corresponding data file. This file would then have to be referenced directly, using path.resolve(…) or require.resolve(…).

  • 👎 Implicitly depends on ./postinstall.js
    • 👍 The file is an implementation detail, relocating it would simply mean updating the "postinstall" script in package.json
  • 👎 The grouped-by data files' locations in node_modules are explicitly mentioned in README: path/to/node_modules/@iso4217/json/data-grouped-by-*.json
    • Investigate if it is possible to use a symlink (./data-grouped-by-*.json./dist/data-grouped-by-*.json)
  • This clumsy context-dependent implementation was a disastrous choice from the start 🤦‍♀️

⛔️ Generating data-grouped-by-*.json files manually

node path/to/node_modules/@iso4217/json/build-grouped-by-data-files.js
  • 👎 Explicitly depends on ./build-grouped-by-data-files.js
  • 👍 Can be substituted with:
    • npm install
      • 👎 which might not work for everybody, because it re-installs everything; e.g., in case when installing dependencies from a private registry, or using custom auth, or making lots of calculations during npm install phase
    • node path/to/node_modules/@iso4217/json/postinstall.js
      • 👎 which is just a shift of dependency
        • 👍 although, in this case, postinstall is a higher abstraction, and the file sits on its place

🔄 Import ISO 4217 data with import statement

import { json } from '@iso4217/json'

🔄 Import JSXml type

import { type JSXml } from '@iso4217/json'

const data: JSXml = /* data */
  • 👎 Implicitly depends on ./index.{js,d.ts}
    • 👍 Can be substituted with "exports"
@parzhitsky parzhitsky added Change: patch [Issue / PR] describes a small non-breaking change, such as bugfix or an optimization Domain: meta [Issue / PR] describes change in the development process, documentation, maintenance etc. Priority: low [Issue / PR] could be addressed at any convenient time Type: improvement [Issue / PR] addresses lack of a functionality or an open possibility of enhancement labels Jul 20, 2023
@parzhitsky parzhitsky added Pending: unclear [Issue] not yet fully defined Domain: main [Issue / PR] describes change in the functionality, its optimization Type: investigation [Issue / PR] addresses the need of gaining intel and removed Change: patch [Issue / PR] describes a small non-breaking change, such as bugfix or an optimization labels Jul 21, 2023
@parzhitsky
Copy link
Member Author

Or just @iso4217/json2 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: main [Issue / PR] describes change in the functionality, its optimization Domain: meta [Issue / PR] describes change in the development process, documentation, maintenance etc. Pending: unclear [Issue] not yet fully defined Priority: low [Issue / PR] could be addressed at any convenient time Type: improvement [Issue / PR] addresses lack of a functionality or an open possibility of enhancement Type: investigation [Issue / PR] addresses the need of gaining intel
Projects
None yet
Development

No branches or pull requests

1 participant