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

refactor: remove hand-written index files #5440

Merged
merged 2 commits into from
May 19, 2020

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented May 15, 2020

  1. The first commit moves if (require.main === module) code from /index.js to src/index.ts. This is a preparation to allow us to remove /index.js file entirely. It leaves the repository in a broken state, but that's fine because the next commit fixes the problem and will be squashed into this one before landing.

    This change has a nice benefit that it enables TypeScript compiler checks for the code building app config, it discovered a possible problem in +process.env.PORT line (+undefined returns NaN).

    I am also slightly changing exports to improve ergonomics - the type ApplicationOptions is exported together with the main application class, so that consumers don't need to import the options from @loopback/context.

  2. In the next step, I am committing the outcome of running an automated script (see https://gist.github.com/bajtos/68672a8229bd03fddad5a4ad1442061f) to remove top-level index files and update package.json metadata.

  3. In the third commit I am updating project templates in packages/cli to match the new style.

  4. Finally I am updating documentation, tutorials and README files in example/*.

Resolves #2613, resolves #2609

Verification

Here are the steps I performed to verify that this change does not break our source-code editing experience in VS Code:

  1. Run npm run clean to remove all dist folders

  2. Verify that VSCode is still able to understand relations between source code artifacts defined in one package (e.g. context) and used elsewhere (e.g. example-todo), as described in https://github.com/strongloop/loopback-next/blob/master/docs/site/VSCODE.md:

    • Verify that "Go to definition" works across package boundaries. Find a place
      where we are calling @inject in authentication package, press F12 to go to
      the definition of inject. VSCode should jump to the .ts file in src (as
      opposed to jumping to a .d.ts file in dist)
    • Verify that refactorings like "rename symbol" (F2) will change all places
      using the renamed entity. Two different scenarios to verify: rename at the place
      where the entity is defined, rename at the place where the entity is used. (You
      can e.g. rename inject to test.)

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users Internal Tooling Issues related to our tooling and monorepo infrastructore labels May 15, 2020
@bajtos bajtos requested a review from raymondfeng May 15, 2020 07:47
@bajtos bajtos self-assigned this May 15, 2020
@bajtos bajtos force-pushed the refactor/remove-dummy-index-files branch 2 times, most recently from 2edfa24 to 84b2d30 Compare May 15, 2020 08:02
@bajtos bajtos mentioned this pull request May 15, 2020
7 tasks
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

👍

Here are the steps I performed to verify that this change does not break our source-code editing experience in VS Code

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

I tried on VScode. It seems to be working.

@raymondfeng
Copy link
Contributor

@bajtos Please investigate why CI is failing.

@raymondfeng raymondfeng force-pushed the refactor/remove-dummy-index-files branch from 84b2d30 to d74b237 Compare May 15, 2020 21:48
@bajtos bajtos force-pushed the refactor/remove-dummy-index-files branch from d74b237 to 834d6cd Compare May 18, 2020 07:48
@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

To be done: update documentation, tutorials and README files in example/*.

Done in 834d6cd. I searched for all occurrences of the string index.js in *.md files to locate places needed an update.

@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

Please investigate why CI is failing.

These failures are weird, I am not able to reproduce them in my working copy. I had to follow up the exact steps performed by Travis CI (starting from git init and git remote add) to get the build to fail locally (albeit with a slightly different message from Prettier).

2e32364 is fixing the problem.

Strangely enough, after fixing Mocha failures, I see linting errors now, in code that seem to be unrelated to my changes 😠

@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

Possibly related: typescript-eslint/typescript-eslint#1573

@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

Strangely enough, after fixing Mocha failures, I see linting errors now, in code that seem to be unrelated to my changes

I think this problem is caused by our code-lint script that runs eslint on the bare repository (without running tsc to build dist files first).

@bajtos bajtos force-pushed the refactor/remove-dummy-index-files branch from 67c8beb to 07b499e Compare May 18, 2020 08:53
@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

I tried to configure eslint to be aware of TypeScript Project References (see 07b499e), but that did not solve the problem :(

See also https://github.com/typescript-eslint/typescript-eslint/blob/795fd1c529ee58e97283c9ddf8463703517b50ab/packages/parser/README.md#parseroptionsproject

@bajtos bajtos force-pushed the refactor/remove-dummy-index-files branch from 07b499e to b88c3d9 Compare May 18, 2020 09:04
@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

@raymondfeng I added three new commits, PTAL:

  • 834d6cd update docs
  • 2e32364 fix yarn test failing on prettier
  • b88c3d9 compile the code before running eslint on Travis CI

I am not very happy about b88c3d9, but then it's affecting only one of the parallel jobs on CI, so it shouldn't make things any worse.

I have some ideas how to improve our eslint setup so that the build step is hopefully not needed, but I prefer to leave them out of scope of this pull request and explore them after this PR is landed.

@bajtos bajtos requested a review from raymondfeng May 18, 2020 11:12
@bajtos
Copy link
Member Author

bajtos commented May 18, 2020

coverage/coveralls — Coverage decreased (-0.5%) to 91.666%

This is caused by the fact that our tests are not executing main functions in src/index.ts. I am surprised they were not detected by nyc/coveralls before my change, while the code was living in index.js files 🤷‍♂️

Screen Shot 2020-05-18 at 13 14 23

bajtos added 2 commits May 19, 2020 08:49
Use `package.json` metadata like `main`, `server` and `types`
to redirect package consumers to `dist/index.js` and `dist/index.d.ts`
instead.

- This is the first commit where I am moving logic from `/index.js`
  to `src/index.js` only.

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos force-pushed the refactor/remove-dummy-index-files branch from b88c3d9 to 8608ea7 Compare May 19, 2020 06:49
@bajtos bajtos merged commit 304d721 into master May 19, 2020
@bajtos bajtos deleted the refactor/remove-dummy-index-files branch May 19, 2020 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable TypeScript Project References
3 participants