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

Remove lib folder (LKG) and use node_modules for building #52226

Merged
merged 20 commits into from
Mar 7, 2023

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jan 13, 2023

Per #52199, this:

  • Makes our build respect --lkg, --built, and no flag, with the default being node_modules.
  • Removes the lib folder.
  • gitignores the lib folder.
  • Removes the bump-lkg-to-nightly task
    • It might be better to just change this to one which automates updating this with --save-exact, which is likely what we will want to use.

Why?

  • This folder bloats the repo; it's about 40MB (after TS 5.0; some 64MB+ before that), and each time it changes, the contents are mostly stored in git as full objects, not as diffs. This also slows down cloning. We can't fix the past, but we can prevent this growth for the future.
  • Since we use external tooling like typescript-eslint, we have to have a copy in node_modules; this version may differ from the LKG and cause differences when typescript-eslint runs the type checker again. The only things you can do are:
    1. Try and keep the code compiling (or at least not erroring) in both; this is "fun", because we have to compile with node_modules, lib, and the built compiler.
    2. Keep LKG and node_modules in sync (Bump LKG and package.json to 5.0.0-dev.20230112 #51787).
    3. Delete LKG (this PR).
  • The folder is checked in, so if one builds LKG locally, loads of changes appear that are unrelated to the actual state of the tree.
    • If you're bisecting TypeScript and want to use npm link, you have to clean/restore lib each time you need to switch commits, as the package is also in lib and therefore is replaced on hereby lkg.
  • Some people try and use lib on main, thinking it actually means something, but it doesn't; it's just whatever random version we are currently using to compile the compiler. It can be arbitrarily out of date. For example, lib contained a copy of ~TS 4.4 for multiple releases.
    • Also, people keep sending us PRs for the lib folder, which a bot then has to monitor and close PRs which attempt to edit the wrong thing.

Downsides

  • When switching branches, bisecting, etc, we'll be more sensitive to node_modules being out of date. Right now, this is already a reality for the core build system, but since LKG was checked in, at least the compiler would always run. Now, if crossing a threshold where node_modules' copy of TypeScript changed, an npm ci will be required.
  • Since lib is gitignored and we still want to check in lib to release branches, we'll have to make sure to force add the dir there.
  • Eventually, we'll probably have "type": "module" in our package.json; we'll need to ensure that "typescript" when referenced in our repo still goes to node_modules and not ourselves.
  • We can't LKG at the same time as making a checker change, if such a thing is needed. I have no idea when this has ever been needed, though. Maybe for new syntax, but given our reliance on esbuild and other tooling, this seems like it's not going to happen.
  • When bisecting past this change, some files may be left in lib. But this should only happen if you regularly run lkg, which already would have left the tree dirty in a more annoying way.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 13, 2023
@jakebailey jakebailey marked this pull request as ready for review March 2, 2023 04:35
@@ -15,6 +15,7 @@ const dest = path.join(root, "lib");

async function produceLKG() {
console.log(`Building LKG from ${source} to ${dest}`);
await fs.mkdirp(dest);
Copy link
Member

Choose a reason for hiding this comment

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

Seems weird that this is also on line 20 but maybe that's intentional; just observing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably the latter can go away; it has to at least come first because del behaves poorly if the root doesn't exist. I'll test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this code is pointless now. I should just delete the dir and recreate it. It won't have a README either.

Copy link
Member Author

@jakebailey jakebailey Mar 6, 2023

Choose a reason for hiding this comment

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

Changed. This was probably a side effect of me hacking to make it work many months ago and then never coming back to it until now, now that this is apparently happening and in the iteration planning 😅

Copy link
Member

Choose a reason for hiding this comment

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

All I really want is a type system for describing whether various make dir / delete dir / copy to dir functions fail/don't fail when the relevant target is already there / not there yet / already deleted 🫠

@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 6, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 8d71f47. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 6, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/148334/artifacts?artifactName=tgz&fileId=C7C587B59FA617ACCEBDC865E84F1F9CC32442777726CA0464AB40E96E3EBE3702&fileName=/typescript-5.1.0-insiders.20230306.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-52226-2".;

@jakebailey
Copy link
Member Author

Well, I've restored the status quo when it comes to package size testing. I'll test out some other options for later but now the PR should keep things going.

@jakebailey
Copy link
Member Author

Seems like it's Go Time. Thanks for reviewing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants