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.

Sign up for GitHub

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

Jump to bottom

fix: Remove devDependencies from RTS build #22239

Merged
merged 4 commits into from
Apr 13, 2023
Merged

fix: Remove devDependencies from RTS build #22239

merged 4 commits into from
Apr 13, 2023

Conversation

ChandanBalajiBP
Copy link
Contributor

@ChandanBalajiBP ChandanBalajiBP commented Apr 10, 2023 •
edited
Loading

Description

Remove devDependencies from the RTS build.
In process of building RTS, we used to copy node_modules to /dist (Build folder)
But this is not ideal process which increases the build size by adding unwanted packages.
Now as part of the solution, we are only moving only prodDependencies to /dist.
This also resolves user request to exclude vulnerable package - formidable.

Fixes #22182

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Manual

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@ChandanBalajiBP
Copy link
Contributor Author

/ok-to-test sha=759f896

@github-actions github-actions bot added the Bug Something isn't working label Apr 10, 2023
@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4659412237.
Workflow: Appsmith External Integration Test Workflow.
Commit: 759f896.
PR: 22239.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=22239&runId=4659412237_1

@github-actions
Copy link

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4659412237.
Commit: 759f896.
The following are new failures, please fix them before merging the PR:


    Identified Flaky tests

    1 similar comment
    @github-actions
    Copy link

    Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4659412237.
    Commit: 759f896.
    The following are new failures, please fix them before merging the PR:


      Identified Flaky tests

      sharat87
      sharat87 requested changes Apr 11, 2023
      View reviewed changes
      Copy link
      Member

      @sharat87 sharat87 left a comment

      Choose a reason for hiding this comment

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

      Thanks @ChandanBalajiBP, looks good, just a few minor changes and we should be ready to push this. Also, two more questions please:

      1. Can you confirm that the formidable package is not present in the node_modules when built this way. I'm sure you checked, just double-confirming.
      2. We've made some changes to the shared AST package.json. Does the client build have any impact too? Is that build working fine?

      app/rts/build.sh Outdated Show resolved Hide resolved
      app/rts/build.sh Outdated
      # Keep copy of all dependencies in node_modules_bkp
      mv node_modules node_modules_bkp
      # Install only production dependencies
      yarn install --production --frozen-lockfile
      tsc_exit_code=$?
      Copy link
      Member

      Choose a reason for hiding this comment

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

      This gets the exit code of the last command, which was tsc-alias, but the last command has changed. In fact, this is actually not needed, let's remove this line, as well as the last exit $tsc_exit_code line. Neither of them are needed.

      Copy link
      Contributor Author

      Choose a reason for hiding this comment

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

      Updated

      @ChandanBalajiBP
      Copy link
      Contributor Author

      /ok-to-test sha=f2587bd

      @github-actions
      Copy link

      Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4664144634.
      Workflow: Appsmith External Integration Test Workflow.
      Commit: f2587bd.
      PR: 22239.
      Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=22239&runId=4664144634_1

      sharat87
      sharat87 previously approved these changes Apr 11, 2023
      View reviewed changes
      @github-actions
      Copy link

      Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4664144634.
      Commit: f2587bd.
      The following are new failures, please fix them before merging the PR:


        Identified Flaky tests

        @ChandanBalajiBP
        Copy link
        Contributor Author

        /ok-to-test sha=762c52e

        @github-actions
        Copy link

        Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4665603870.
        Workflow: Appsmith External Integration Test Workflow.
        Commit: 762c52e.
        PR: 22239.
        Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=22239&runId=4665603870_1

        @github-actions
        Copy link

        Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4665603870.
        Commit: 762c52e.
        The following are new failures, please fix them before merging the PR:

        1. cypress/integration/Regression_TestSuite/ClientSideTests/Autocomplete/Autocomplete_JS_spec.ts

        2. cypress/integration/Regression_TestSuite/ClientSideTests/Widgets/JSONForm/JSONFrom_Modal_spec.js
        Identified Flaky tests

        @github-actions
        Copy link

        Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4665603870.
        Commit: 762c52e.
        All cypress tests have passed 🎉

        1 similar comment
        @github-actions
        Copy link

        Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/4665603870.
        Commit: 762c52e.
        All cypress tests have passed 🎉

        @ChandanBalajiBP
        Copy link
        Contributor Author

        Adding test plan approved as the Ci has passed and from the build folder there are no dev dependencies.

        @ChandanBalajiBP ChandanBalajiBP added the Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable label Apr 13, 2023
        @github-actions github-actions bot added AST-frontend Issues related to maintaining AST logic Query & JS Pod Issues related to the query & JS Pod Data Platform Pod Issues related to the underlying data platform Entity Refactor Issues related to refactor logic Javascript Product Issues related to users writing javascript in appsmith Needs Triaging Needs attention from maintainers to triage labels Apr 13, 2023
        @sharat87 sharat87 merged commit 88da94a into release Apr 13, 2023
        @sharat87 sharat87 deleted the fix/remove-dev-dependencies-from-rts branch April 13, 2023 12:02
        @Nikhil-Nandagopal Nikhil-Nandagopal removed the Data Platform Pod Issues related to the underlying data platform label Aug 5, 2024
        Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
        Reviewers

        @sharat87 sharat87 sharat87 approved these changes

        Assignees
        No one assigned
        Labels
        AST-frontend Issues related to maintaining AST logic Bug Something isn't working Entity Refactor Issues related to refactor logic Javascript Product Issues related to users writing javascript in appsmith Needs Triaging Needs attention from maintainers to triage Query & JS Pod Issues related to the query & JS Pod Test Plan Approved Manual/Cypress tests covers changes made on the PR. Else, add skip-testPlan label if not applicable
        Projects
        None yet
        Milestone
        No milestone
        Development

        Successfully merging this pull request may close these issues.

        [Bug]: Remove devDependencies from the RTS build
        3 participants
        @ChandanBalajiBP @sharat87 @Nikhil-Nandagopal