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

feat(build): packagesManager and PackageMetadata abstractions & integration tests #5940

Closed
wants to merge 1 commit into from

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Apr 22, 2024

Extracted from #5876 to hopefully land it in smaller chunks. Addresses the infrastructure needed for #5869 (without the associated changes to scripts).

  • Adds some tests to sanity check packages (their package.json, the existence of flow and docs)
  • Generate boilerplate README.md and docs/packages/{package}.md files (npm run update-docs)
  • Adds build integration tests to make sure all of the examples build and run with release artifacts

This does not make any major changes to any of the other build infrastructure, I will push those PRs separately if this is reviewed & merged instead of #5876.

I've also removed lint changes and the additional build integration tests for astro, svelte and next.js to minimize this PR.

If you'd rather get this done in one go, review and merge #5876 instead which includes these changes.

Copy link

vercel bot commented Apr 22, 2024

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

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2024 7:21pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2024 7:21pm

@etrepum
Copy link
Collaborator Author

etrepum commented Apr 23, 2024

@StyleT here's an attempt to break #5876 up. This is almost entirely an additive change (except for the small changes required for eslint rules and tsc compliance). I could move the build integration tests (basically just boilerplate projects to show that the build works with various frameworks) into their own PR if this needs to be even smaller.

@@ -0,0 +1,29 @@
{
"name": "lexical-esm-astro-react",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this and other new packages be here? If yes - why do they have actual implementation instead of being a simple stubs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are integration tests that see if lexical can be consumed by other projects with various bundlers and frameworks. It needs to be a whole project in order to demonstrate integration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the meantime I have removed these fixtures so there are fewer files to look at in the interest of expediting this

.eslintrc.js Outdated
// This isn't useful in our test code
'react/react-in-jsx-scope': ERROR,
// This hasn't been necessary since React 17
'react/react-in-jsx-scope': OFF,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't seem related to build scripts. As well as the some other (not scoped to /scripts/) within this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can back these out if it helps, I had modified these lint rules because they have not served a purpose in years and it caused problems for some of the other frameworks before I had ignored them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I backed out these lint rule changes and the fixtures to make this smaller

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the reason I had made those lint changes in the first place:

❯ (cd examples/react-rich; npm run build)

> @lexical/react-rich-example@0.14.5 build
> tsc && vite build

src/App.tsx:14:1 - error TS6133: 'React' is declared but its value is never read.

14 import * as React from 'react';
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/ToolbarPlugin.tsx:22:1 - error TS6133: 'React' is declared but its value is never read.

22 import * as React from 'react';
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/ToolbarPlugin.tsx:60:20 - error TS6133: 'newEditor' is declared but its value is never read.

60         (_payload, newEditor) => {
                      ~~~~~~~~~

src/plugins/TreeViewPlugin.tsx:11:1 - error TS6133: 'React' is declared but its value is never read.

11 import * as React from 'react';
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 4 errors in 3 files.

Errors  Files
     1  src/App.tsx:14
     2  src/plugins/ToolbarPlugin.tsx:22
     1  src/plugins/TreeViewPlugin.tsx:11

Comment on lines +13 to +18
"@lexical/html": ["../lexical-html/src/"],
"@lexical/link": ["../lexical-link/src/"],
"@lexical/mark": ["../lexical-mark/src/"],
"@lexical/selection": ["../lexical-selection/src/"],
"@lexical/table": ["../lexical-table/src/"],
"@lexical/utils": ["../lexical-utils/src/"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

npm run ci-check does not pass for me without this because npm run tsc-extension will fail with resolution errors

});
}
});
['examples', 'scripts/__tests__/integration/fixtures']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the additional fixtures from this PR

@StyleT
Copy link
Contributor

StyleT commented Apr 23, 2024

Glad to see this got smaller. I'll take another look today/tomorrow ;) Let's not forget main purpose of this: simplify new package addition. Hope these changes serve the purpose well ;)

@etrepum
Copy link
Collaborator Author

etrepum commented Apr 23, 2024

This is mostly just the infrastructure to make that possible. #5876 includes the actual work to make it easier to add a new package as it requires sweeping changes to all sorts of build and configuration so that new packages get picked up.

@etrepum
Copy link
Collaborator Author

etrepum commented Apr 23, 2024

A larger PR is preferable for me because as an external contributor I have no way to submit stacked diffs for review. Landing this work in pieces would take months and would not really help anyone add a new package in the meantime.

Comment on lines +15 to +30

const increment = argv.i;
const validIncrements = new Set(['minor', 'patch', 'prerelease']);
if (!validIncrements.has(increment)) {
console.error(`Invalid value for increment: ${increment}`);
process.exit(1);
}

async function incrementVersion(increment) {
async function incrementVersion() {
const preId = increment === 'prerelease' ? '--preid next' : '';
const workspaces = '';
const command = `npm --no-git-tag-version version ${increment} --include-workspace-root true ${preId} ${workspaces}`;
await exec(command);
}

incrementVersion(increment);
incrementVersion();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just compliance with the monorepo lint rules. Previously this directory was included with overly general ignore rules because the folder happened to be named npm.

…s and build integration tests

Extracted from facebook#5876 to hopefully land it in smaller chunks.
Addresses the infrastructure needed for facebook#5869 (without the associated changes to scripts)
@etrepum
Copy link
Collaborator Author

etrepum commented Apr 23, 2024

Closing in favor of the larger #5876

@etrepum etrepum closed this Apr 23, 2024
@etrepum etrepum deleted the packages-manager branch May 11, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants