Bug 1761485 - Expose ErrorType through its own entry point. #1318

tawahpeggy · 2022-04-19T22:15:53Z

Pull Request checklist

Quality: Make sure this PR builds and runs cleanly.
- Inside the glean/ folder, run:
  - npm run test Runs all tests
  - npm run lint Runs all linters
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work
- Documentation should be added to The Glean Book when making changes to Glean's user facing API
  - When changes to the Glean Book are necessary, link to the corresponding PR on the mozilla/glean repository
- Documentation should be added to the Glean.js developer documentation when making internal code changes

Dexterp37

Hi @tawahpeggy and thank you for your contribution!

This needs a few changes before we can merge it: because you're using an old version of npm, the lock files have been rewritten in an old format, making CI fail.

In addition to the above, would you kindly add an entry for this change at the top of the CHANGELOG.md file?

Dexterp37 · 2022-04-20T09:32:35Z

glean/package-lock.json

  "requires": true,
-  "packages": {


Looks like you're using an outdated npm and the lockfile was rewritten with an older version. Would you kindly update your local npm and drop this change?

@tawahpeggy this was fully removed instead of restored, would you kindly restore the file?

glean/package.json

glean/tests/unit/platform/utils/webext/sample/package-lock.json

tawahpeggy · 2022-04-21T09:16:38Z

Thank you very much for your review, will update the PR ASAP

glean/package.json

CHANGELOG.md

tawahpeggy · 2022-04-21T11:14:11Z

Hello @Dexterp37, please do you have an idea why some tests fail?

Dexterp37 · 2022-04-21T11:42:17Z

Hello @Dexterp37, please do you have an idea why some tests fail?

It doesn't seem to be able to import ErrorType. I believe the imports need to be adjusted :)

TSError: ⨯ Unable to compile TypeScript:
tests/unit/index.test.ts:6:10 - error TS2614: Module '"@mozilla/glean/webext"' has no exported member 'ErrorType'. Did you mean to use 'import ErrorType from "@mozilla/glean/webext"' instead?

6 import { ErrorType } from "@mozilla/glean/webext";
           ~~~~~~~~~

    at createTSError (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:820:12)
    at reportTSError (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:824:19)
    at getOutput (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:1014:36)
    at Object.compile (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:1322:43)
    at transformSource (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/esm.ts:380:37)
    at load (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/esm.ts:280:51)
    at async ESMLoader.load (node:internal/modules/esm/loader:359:20)
    at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:280:47)
    at async link (node:internal/modules/esm/module_job:70:21)

Exited with code exit status 1

CircleCI received exit code 1

tawahpeggy · 2022-04-21T13:49:20Z

Hello @Dexterp37, please do you have an idea why some tests fail?

It doesn't seem to be able to import ErrorType. I believe the imports need to be adjusted :)

TSError: ⨯ Unable to compile TypeScript:
tests/unit/index.test.ts:6:10 - error TS2614: Module '"@mozilla/glean/webext"' has no exported member 'ErrorType'. Did you mean to use 'import ErrorType from "@mozilla/glean/webext"' instead?

6 import { ErrorType } from "@mozilla/glean/webext";
           ~~~~~~~~~

    at createTSError (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:820:12)
    at reportTSError (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:824:19)
    at getOutput (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:1014:36)
    at Object.compile (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/index.ts:1322:43)
    at transformSource (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/esm.ts:380:37)
    at load (/home/circleci/project/samples/browser/webext/node_modules/ts-node/src/esm.ts:280:51)
    at async ESMLoader.load (node:internal/modules/esm/loader:359:20)
    at async ESMLoader.moduleProvider (node:internal/modules/esm/loader:280:47)
    at async link (node:internal/modules/esm/module_job:70:21)

Exited with code exit status 1

CircleCI received exit code 1

Hmmmmm this is strange and I do not even know what I have done wrong.
everything is underlined in the test file :sad: .

Dexterp37 · 2022-04-27T16:58:02Z

Hmmmmm this is strange and I do not even know what I have done wrong. everything is underlined in the test file :sad: .

In this PR you're changing the way the ErrorType symbol is being exported. Without this PR, things are being exported via the platform-specific endpoints, e.g. import { ErrorType } from "@mozilla/glean/webext";.
This PR exposes it through its own entry point and so call sites need to be adjusted like this:

import { ErrorType } from "@mozilla/glean/error";

Dexterp37 · 2022-04-28T12:39:26Z

@tawahpeggy note that the proper import need to be applied to all the samples and in all the other locations within the codebase.

Dexterp37 · 2022-04-29T10:07:01Z

glean/package.json

  "type": "module",
  "sideEffects": false,
-  "exports": {
+  "exports": {    


nit: please remove the trailing whitespaces on this line

glean/src/core/error/index.ts

Dexterp37

We're almost there! I think with the last two requested changes it should be ok for merging.

glean/src/core/error/index.ts

glean/src/entry/qt.ts

tawahpeggy · 2022-05-10T10:15:49Z

Hello @Dexterp37, hope you are having a great day.
please help check this PR and see if there's something else to be done

Dexterp37 · 2022-05-11T09:21:09Z

Hello @Dexterp37, hope you are having a great day. please help check this PR and see if there's something else to be done

Hi @tawahpeggy , I'm doing great, thanks. Note that the PR is still failing the tests (see the failing CI jobs), so we can't merge it: would you kindly fix the codebase to make sure tests run ok? You can also run them locally to reduce the turnaround time.

tawahpeggy · 2022-05-18T14:47:12Z

Hello @Dexterp37, hope you are having a great day. please help check this PR and see if there's something else to be done

Hi @tawahpeggy , I'm doing great, thanks. Note that the PR is still failing the tests (see the failing CI jobs), so we can't merge it: would you kindly fix the codebase to make sure tests run ok? You can also run them locally to reduce the turnaround time.

I noticed the failed test but I really do not know how to fix it. any heads up?

Dexterp37 · 2022-05-23T10:41:05Z

Hello @Dexterp37, hope you are having a great day. please help check this PR and see if there's something else to be done

Hi @tawahpeggy , I'm doing great, thanks. Note that the PR is still failing the tests (see the failing CI jobs), so we can't merge it: would you kindly fix the codebase to make sure tests run ok? You can also run them locally to reduce the turnaround time.

I noticed the failed test but I really do not know how to fix it. any heads up?

I think we need to add something like this

      "error": [
        "./dist/types/core/error/error_type.d.ts"
      ]

after "testing" in here

tawahpeggy · 2022-06-17T12:33:14Z

Thank you @Dexterp37,
I get this message at the end. when I run the test, please is it something I should worry about?

Dexterp37 · 2022-06-20T08:56:10Z

Thank you @Dexterp37, I get this message at the end. when I run the test, please is it something I should worry about?

I don't think we should worry about that as long as this runs on CI. Let me check :)

moz-glean · 2022-06-20T09:14:43Z

Build size report

Merging #1318 into main will:

Leave the size of full Web Extension bundle unchanged.
Leave the size of full Website bundle unchanged.
Leave the size of full Node.js bundle unchanged.
Leave the size of full Qt/QML bundle unchanged.

	Current size	New size	Size increase
Web Extension
core only	55 KB	55 KB	📉 0 bytes
full bundle	95 KB	95 KB	📉 0 bytes
Website
core only	56 KB	56 KB	📉 0 bytes
full bundle	96 KB	96 KB	📉 0 bytes
Node.js
core only	55 KB	55 KB	📉 23 bytes
full bundle	101 KB	101 KB	📉 23 bytes
Qt/QML
core only	78 KB	78 KB	📉 0 bytes
full bundle	78 KB	78 KB	📉 0 bytes

Dexterp37 · 2022-06-20T09:17:10Z

Thank you @tawahpeggy ! This is now in a good shape, I'll merge it and close the related bug.

tawahpeggy · 2022-06-21T17:47:44Z

Thank you for merging,and thank you for being a great mentor. I noticed you have closed the bug but i still have to add the documentation to the glean book. Will be working on it as soon as possible.

tawahpeggy · 2022-06-21T17:48:47Z

Thank you @Dexterp37, I get this message at the end. when I run the test, please is it something I should worry about?

I don't think we should worry about that as long as this runs on CI. Let me check :)

Okay understood thanks.

Dexterp37 · 2022-06-22T09:36:44Z

Thank you for merging,and thank you for being a great mentor. I noticed you have closed the bug but i still have to add the documentation to the glean book. Will be working on it as soon as possible.

Thank you for helping out with this! Good catch, I completely forgot about the docs updates. I re-opened the bug.

auto-assign bot requested a review from chutten April 19, 2022 22:15

badboy requested a review from Dexterp37 April 20, 2022 08:17

Dexterp37 removed the request for review from chutten April 20, 2022 09:34

Dexterp37 assigned tawahpeggy Apr 20, 2022

Dexterp37 requested changes Apr 20, 2022

View reviewed changes

Dexterp37 requested changes Apr 21, 2022

View reviewed changes

glean/package.json Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

tawahpeggy changed the title ~~Bug 1761485 - Expose ErrorType through its own entry point.~~ WIP: Bug 1761485 - Expose ErrorType through its own entry point. Apr 28, 2022

Bug1761485-expose-ErrorType-through-its-own-entry-point

912760a

tawahpeggy force-pushed the Bug1761485-expose-ErrorType-through-its-own-entry-point branch from c00ac16 to 912760a Compare April 28, 2022 22:49

tawahpeggy added 6 commits April 29, 2022 00:08

Bug1761485-expose-ErrorType-through-its-own-entry-point

9fa4b1d

Bug1761485-expose-ErrorType-through-its-own-entry-point

919a83d

Bug1761485-expose-ErrorType-through-its-own-entry-point

9b2cb27

Bug1761485-expose-ErrorType-through-its-own-entry-point

4876aae

Bug1761485-expose-ErrorType-through-its-own-entry-point

35dc191

Bug1761485-expose-ErrorType-through-its-own-entry-point

0bda926

Dexterp37 requested changes Apr 29, 2022

View reviewed changes

tawahpeggy added 2 commits April 29, 2022 14:12

Bug1761485-expose-ErrorType-through-its-own-entry-point

6a992a8

Bug1761485-expose-ErrorType-through-its-own-entry-point

eabf17b

Dexterp37 requested changes May 2, 2022

View reviewed changes

glean/src/core/error/index.ts Outdated Show resolved Hide resolved

glean/src/entry/qt.ts Outdated Show resolved Hide resolved

Bug1761485-expose-ErrorType-through-its-own-entry-point

e2e5866

tawahpeggy changed the title ~~WIP: Bug 1761485 - Expose ErrorType through its own entry point.~~ Bug 1761485 - Expose ErrorType through its own entry point. May 6, 2022

Bug 1761485 - Expose ErrorType through its own entry point.

4ac629a

Dexterp37 closed this Jun 20, 2022

Dexterp37 reopened this Jun 20, 2022

Dexterp37 approved these changes Jun 20, 2022

View reviewed changes

Dexterp37 merged commit bc83a49 into mozilla:main Jun 20, 2022

Bug 1761485 - Expose ErrorType through its own entry point. #1318

Bug 1761485 - Expose ErrorType through its own entry point. #1318

Uh oh!

Conversation

tawahpeggy commented Apr 19, 2022

Pull Request checklist

Uh oh!

Dexterp37 left a comment

Choose a reason for hiding this comment

Uh oh!

Dexterp37 Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

Dexterp37 Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tawahpeggy commented Apr 21, 2022

Uh oh!

Uh oh!

Uh oh!

tawahpeggy commented Apr 21, 2022

Uh oh!

Dexterp37 commented Apr 21, 2022

Uh oh!

tawahpeggy commented Apr 21, 2022

Uh oh!

Dexterp37 commented Apr 27, 2022

Uh oh!

Dexterp37 commented Apr 28, 2022

Uh oh!

Dexterp37 Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dexterp37 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tawahpeggy commented May 10, 2022

Uh oh!

Dexterp37 commented May 11, 2022

Uh oh!

tawahpeggy commented May 18, 2022

Uh oh!

Dexterp37 commented May 23, 2022

Uh oh!

tawahpeggy commented Jun 17, 2022

Uh oh!

Dexterp37 commented Jun 20, 2022

Uh oh!

moz-glean commented Jun 20, 2022

Build size report

Uh oh!

Dexterp37 commented Jun 20, 2022

Uh oh!

tawahpeggy commented Jun 21, 2022

Uh oh!

tawahpeggy commented Jun 21, 2022

Uh oh!

Dexterp37 commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants