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

bump to @bentley/react-scripts@4.0.3, eslint@^7.11.0 #942

Merged
merged 22 commits into from
Jul 2, 2021
Merged

Conversation

aruniverse
Copy link
Member

Bumping @bentley/react-scripts to 4.0.3, see imodeljs/create-react-app#35

Moved some of the flags from the package.json scripts into an .env file, users can override it with a .env.*.local file with different setting for development or production builds in a .env.development or .env.production file

The core monorepo is using react@16.14.0 which supports the new react-jsx transform, but we couldn't use it until typescript was bumped to 4.1 or we bumped react-scripts to 4.0.0. I have explicitly disabled the use of this new transform, but we can re-enable later. Currently there is an issue using that feature because we pin down our @types/react down to 16.9.43 and its missing the type for the transform. "Could not find a declaration file for module 'react/jsx-runtime'"

Eitherway, with this change, everything still compiles. Due to outstanding issues with the test-apps w.r.t breaking changes in 2.13.x i was unable to test it out thoroughly, but everything should be fine.

@pmconne
Copy link
Member

pmconne commented Mar 11, 2021

Due to outstanding issues with the test-apps w.r.t breaking changes in 2.13.x i was unable to test it out thoroughly, but everything should be fine.

Can you elaborate? Are the test-apps broken in some way?

Copy link
Member

@calebmshafer calebmshafer left a comment

Choose a reason for hiding this comment

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

@aruniverse before we merge this, we'll want to make sure to merge the CRA 4 PR in our fork and get an official version published. I'll review that later today.

test-apps/display-test-app/package.json Show resolved Hide resolved
test-apps/ui-test-app/package.json Show resolved Hide resolved
@aruniverse
Copy link
Member Author

aruniverse commented Mar 11, 2021

Due to outstanding issues with the test-apps w.r.t breaking changes in 2.13.x i was unable to test it out thoroughly, but everything should be fine.

Can you elaborate? Are the test-apps broken in some way?

I believe there's an issue with auth in ui-test-app that @ramanujam-raman is actively working on. There's also the breaking electron rpc changes changes in 2.13

Paul i did test the display-test-app, i was able to open a local model, and everything seemed to work, but not too familiar with the app to test it thoroughly.

Copy link
Member

@pmconne pmconne left a comment

Choose a reason for hiding this comment

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

Approving display-test-app changes...can't comment on CRA stuff.

@aruniverse
Copy link
Member Author

I'm going to create an issue with the CopyWebpackPlugins repo once i get back next week, but heres my findings so far as to why the new CopyStaticAssetsPlugin isn't working with pnpm workspaces, and part of a draft of what that issue will look like. (just to refresh my memory next week, posting here in advance and also because i need more time to draft the issue and im out of time for the day :( )


My rush based monorepo just switched to using pnpm workspaces, and the CopyWebpackPlugin didn't work due to dependencies being hoisted, and it's only partially resolved by using require.resolve.

Previously my config looked like:

new CopyPlugin({
  patterns: [
    {
      from: '**/public/**/*',
      context: 'node_modules/@MY_SCOPE',
      noErrorOnMissing: true,
      to({ absoluteFilename }: { absoluteFilename: string }) {
        const regex = new RegExp('(public(?:\\\\|\/))(.*)');
        return regex.exec(absoluteFilename)![2];
      },
    },
    ...
  ],
}),

Basically I have a few (15-20) packages (some with different scopes) that deliver some assets I need for my app. I would previously look in node_modules and try to copy over everything in those package's public dir into my apps public dir.

With the hoisting now its unable to correctly find those dependencies and the assets never get copied over.

The require.resolve does work and I'm able to resolve the path to where the dependency gets hoisted too, but now I need to manually specify all the deps to look for, where as previously I just needed to specify the scope. This is a minor inconvenience and i have a workaround like so:

const appPackageJson = require(paths.appPackageJson);
const pkgsICareAbout = Object.keys(appPackageJson.dependencies).filter(n => n.startsWith('@scope_1') || n.startsWith('@scope_2'));
const copyPatterns = [];
for (const pkg of pkgsICareAbout) {
  copyPatterns.push({
    from: `${path.dirname(require.resolve(`${pkg}/package.json`))}/**/public/**/*`,
    noErrorOnMissing: true,
    to({ absoluteFilename }: { absoluteFilename: string }) {
      const regex = new RegExp('(public(?:\\\\|\/))(.*)');
      return regex.exec(absoluteFilename)![2];
    },
  })
}

But my main issue with this is the glob pattern's dont work.

For example, heres a snippet of what my node_modules looks like with the symlinks:
image
image
The assets I want from this package are found in ui-framework/lib (this is what would show up in a regular node_module install), but that ui-framework pkg exists in my monorepo, so the entire src dir shows up instead of just the lib output dir as you'd see in a regular install.

But eh, i know for a fact that most assets i want will be in a llib, build, esm, or cjs, dir, so i can specify all those folders as well in my copyPatterns array.

But this still doesn't work as its unable to correctly parse the glob pattern and won't copy a single thing over from the public subdir of those pkgs. I need to manually specify the file to copy over and it works, but its way too tedious to make a list of all the files in the public subdir to add to my copyPatterns array.

          {
            from: `${path.dirname(require.resolve(`@bentley/ui-framework/package.json`))}/lib/public/locales/en/UiFramework.json`,
            noErrorOnMissing: true,
            to({ absoluteFilename }) {
              console.log(absoluteFilename)
              return Promise.resolve(
                /(public\\)(.*)/.exec(absoluteFilename)[2]
              );
            },
          },

@aruniverse aruniverse marked this pull request as draft June 16, 2021 21:28
.vscode/settings.json Outdated Show resolved Hide resolved
@aruniverse aruniverse marked this pull request as ready for review June 29, 2021 18:59
@aruniverse aruniverse requested review from calebmshafer and removed request for abeesh June 29, 2021 20:20
@aruniverse aruniverse changed the title bump to imodeljs cra 4.0 fork bump to @bentley/react-scripts@4.0.3, eslint@^7.11.0 Jun 29, 2021
@calebmshafer calebmshafer merged commit b6cfc16 into master Jul 2, 2021
@calebmshafer calebmshafer deleted the cra-4 branch July 2, 2021 01:44
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.

6 participants