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(build): typescript + support inner exports #1336

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

timofei-iatsenko
Copy link
Collaborator

Fix size-limit issue
And fix bundle sizr issue introduced by @message-format/parser exported from @lingui/core

Refactor and rewrite to typescript build scripts

@vercel
Copy link

vercel bot commented Jan 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 16, 2023 at 1:15PM (UTC)

@timofei-iatsenko timofei-iatsenko changed the title Refactor/build refactor(build): typescript + support inner exports Jan 12, 2023
@Martin005 Martin005 changed the title refactor(build): typescript + support inner exports refactor(build): typescript + support inner exports Jan 12, 2023
@timofei-iatsenko
Copy link
Collaborator Author

Still failing, that strange

@timofei-iatsenko
Copy link
Collaborator Author

It worked with flat exports, and started failing again because of multi-entry, probable i've created inner entry incorrectly

@timofei-iatsenko
Copy link
Collaborator Author

🎉 builds! Size testing failing because it's trying to compare with master and master has a failing code now.

@Martin005
Copy link
Contributor

@thekip
I have encountered several issues with your implementation:

  1. After I built the library and released it locally with the /scripts/verdaccio-release.js script, I tried installing it in the current example create-react-app and running with yarn start, but I got the following error:
node:internal/modules/cjs/loader:535
      throw e;
      ^

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /home/martin/js-lingui/examples/create-react-app/node_modules/@lingui/conf/package.json
  1. Following the CONTRIBUTING.md:
  • When I run release:test, I get the following error:
node:internal/modules/cjs/loader:998
  throw err;
  ^

Error: Cannot find module '/home/martin/js-lingui/scripts/build'
  • Also, when I run tsc (as a part of the lint:all command in the main package.json), I get the following error:
$ /home/martin/js-lingui/node_modules/.bin/tsc
packages/core/build/cjs/dev/index.d.ts:1:10 - error TS2305: Module '"../compile/compileMessage"' has no exported member 'compileMessage'.

1 import { compileMessage as compile } from "../compile/compileMessage";
           ~~~~~~~~~~~~~~

packages/core/build/esm/dev/index.d.ts:1:10 - error TS2305: Module '"../compile/compileMessage"' has no exported member 'compileMessage'.

1 import { compileMessage as compile } from "../compile/compileMessage";
           ~~~~~~~~~~~~~~

@andrii-bodnar
Copy link
Contributor

We'd like to have a separate fix for the bug in main and separately the rest (just to not block the new version release)

@timofei-iatsenko
Copy link
Collaborator Author

timofei-iatsenko commented Jan 13, 2023

We'd like to have a separate fix for the bug in main and separately the rest (just to not block the new version release)

it's not possible to ship it separately, to fix the main building system should be changed.
You can disable or ignore size-limit issue for now and ship without it, code should work, it's just an issue with workflow in the monorepo.

Also, when I run tsc (as a part of the lint:all command in the main package.json), I get the following error:

Why it's not failing in CI?

@andrii-bodnar
Copy link
Contributor

@thekip what exactly went wrong in the #1328 that broke this size-check? It might be easier to rollback this change than dangerously change the build system.

It worked before and it's not a good idea to ignore this.

@timofei-iatsenko
Copy link
Collaborator Author

so in short
this is a first time usage of the cross-linked library in client-side code
There are also lingui/react which states that uses core package, but actually it uses only typings, so in real js code imports to core are stripped
and currently when webpack starts resolving imports it goes to ./packages/core/ instead of ./package/core/dst because this is how yarn workspace linking works
When we deploy packages, it deployed from ./dst
but when thy used locally, they used from ./ {root of package}
it means the changes should be bigger to support this
i will try to find a quick workaround for that

@timofei-iatsenko
Copy link
Collaborator Author

In my opinion it's better to "dangerously" change the build system (which in fact is not dangerously)

@timofei-iatsenko
Copy link
Collaborator Author

release:test and lint:all are fixed.

Actually release:test are useless today, it's doing the same thing as in CI but with diffrent code. So issue was in file itself not in tests.

Typings was failing because i marked compileMessage as @internal, and typescript stripped the typings of it. (that's exactly what i wanted to achieve).

I deleted /dev folder with all content and reorganized it in different way. Probably previously (prior v3) the dev was somehow exposed from the package, but right now in current main code it's no longer exposed, and users have no chance to use anything from there, so it's safe.

Looking into issue with No "exports" with @lingui/conf

@timofei-iatsenko
Copy link
Collaborator Author

Fixed all issues, now it works in examples. But came up with better (less fragile) idea, i will try to implement it and look what i get

- publish packages from package root instead of ./build
- remove all manipulation code which move files to the build
- change package.json to point to right sources
- added inner export targets for packages where it were used
- push compileMessage as inner package of `core`
- change build system to be able to build separate entries in multi-entry packages
@timofei-iatsenko
Copy link
Collaborator Author

@Martin005 @andrii-bodnar was managed to simplify process and remove fragile parts. If you're afraid of changes, we can discuss how the process previously looks like, and how it's looks now.

In short:

We do not copy package.json from root of package to ./build, instead we publish package from the package root. This is how it's done in most of the projects on the market. This simplifies things a lot and fix issues with size-check among with others.

scripts/build/packaging.ts Outdated Show resolved Hide resolved
scripts/build/packaging.ts Outdated Show resolved Hide resolved
scripts/build/utils.ts Outdated Show resolved Hide resolved
scripts/build/utils.ts Outdated Show resolved Hide resolved
scripts/build/rollup.ts Outdated Show resolved Hide resolved
@timofei-iatsenko
Copy link
Collaborator Author

Fixed

@andrii-bodnar andrii-bodnar merged commit 830837a into lingui:main Jan 16, 2023
@timofei-iatsenko timofei-iatsenko deleted the refactor/build branch January 16, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants