Skip to content

@joystream/types: remove need for lib from import path#785

Merged
mnaamani merged 6 commits intoJoystream:nicaeafrom
mnaamani:types-remove-lib-from-import-path
Jun 25, 2020
Merged

@joystream/types: remove need for lib from import path#785
mnaamani merged 6 commits intoJoystream:nicaeafrom
mnaamani:types-remove-lib-from-import-path

Conversation

@mnaamani
Copy link
Member

  • Finally getting rid of need to have /lib/ in the import path.
  • Enabled forceConsistentCasingInFileNames tsconfig option which
    "Disallow inconsistently-cased references to the same file"
    This option would have detected the issue we once had on OSX Pioneer: Fix for #511 #514
  • Updated cli and tests in nicaea branch to use new path

@mnaamani mnaamani marked this pull request as draft June 24, 2020 15:52
@mnaamani
Copy link
Member Author

Doing a test by packing the package with npm pack when I try to install it, it actually runs build script, and I'm getting some errors:

$ npm run build
npm WARN lifecycle The node binary used for scripts is /var/folders/x1/jzcg_82x2yndlklx7ystth580000gn/T/yarn--1593013889109-0.3527586406031573/node but npm is using /usr/local/Cellar/node@12/12.18.0/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

> @joystream/types@0.11.0 build /Users/mokhtar/tmp/package
> tsc --build tsconfig.json

node_modules/@polkadot/types/types.d.ts:1:29 - error TS2307: Cannot find module '@polkadot/keyring/types' or its corresponding type declarations.

1 import { SignOptions } from '@polkadot/keyring/types';
                              ~~~~~~~~~~~~~~~~~~~~~~~~~

src/versioned-store/index.ts:8:39 - error TS7016: Could not find a declaration file for module 'lodash'. '/Users/mokhtar/tmp/package/node_modules/lodash/lodash.js' implicitly has an 'any' type.
  Try `npm install @types/lodash` if it exists or add a new declaration (.d.ts) file containing `declare module 'lodash';`

8 import { camelCase, upperFirst } from 'lodash'
                                        ~~~~~~~~

src/media.ts:8:46 - error TS2307: Cannot find module '@polkadot/keyring' or its corresponding type declarations.

8 import { encodeAddress, decodeAddress } from '@polkadot/keyring';
                                               ~~~~~~~~~~~~~~~~~~~


Found 3 errors.

Will try to resolve

@mnaamani
Copy link
Member Author

Fixed by adding missing dependencies in 26c008f

@mnaamani mnaamani marked this pull request as ready for review June 24, 2020 20:25
Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

Seems to work fine after removing node_modules inside the root and project folders and running yarn install.
Tested CLI, Pioneer and Network Tests.
Also tried using the tarball created with npm pack within the CLI.
Didn't run into any issues that I could track to changes introduced in this PR.

Merging this will require updates in: #707 (where some additional imports using the old /lib path were included)

types/.gitignore Outdated
# Don't track build artifacts
**/*.js
**/*.d.ts
hiring/schemas
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is inlcuded as part of .gitignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it to ignore the role.schema.json which is moved into there because its imported by src/hiring/index.ts

import * as role_schema_json from './schemas/role.schema.json'

I will be more explicit and make it ignore that exact file.

@mnaamani mnaamani requested a review from Lezek123 June 25, 2020 14:56
Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

I confused hiring/schemas with src/hiring/schemas, now it makes sense, I think it's good to go then.

@mnaamani mnaamani merged commit 32a3392 into Joystream:nicaea Jun 25, 2020
@mnaamani mnaamani deleted the types-remove-lib-from-import-path branch June 26, 2020 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants