Skip to content

Update query-string to v8.1.0 from v7.1.3#5892

Closed
tsmock wants to merge 1 commit into
hotosm:developfrom
facebook:chore/update-query-string
Closed

Update query-string to v8.1.0 from v7.1.3#5892
tsmock wants to merge 1 commit into
hotosm:developfrom
facebook:chore/update-query-string

Conversation

@tsmock
Copy link
Copy Markdown
Contributor

@tsmock tsmock commented Jun 7, 2023

The major change is that it is now pure ESM and no longer exports specific functions. It also requires Node.js 14+.

@github-actions github-actions Bot added scope: frontend dependencies Pull requests that update a dependency file javascript labels Jun 7, 2023
@tsmock tsmock force-pushed the chore/update-query-string branch from 4658c2d to d8a674b Compare June 7, 2023 19:19
@HelNershingThapa
Copy link
Copy Markdown
Contributor

This babel-loader problem occurred to me. Does it compile successfully at your end?

Failed to compile.

./node_modules/query-string/base.js 422:14
Module parse failed: Unexpected token (422:14)
File was processed with these loaders:
 * ./node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
| 
|   return {
>     url: url_?.split('?')?.[0] ?? '',
|     query: parse(extract(url), options),
|     ...(options && options.parseFragmentIdentifier && hash ? {

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 9, 2023

It did. But I hadn't wiped the node_modules directory, so maybe there was a required component there. I'll debug and (probably) push a new yarn.lock file.

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 9, 2023

Well, rm -rf node_modules && yarn install && yarn build worked for me.

Maybe there is a version issue? My tool versions are:

  • nodejs: 16.20.0
  • yarn: 1.22.19

@eternaltyro
Copy link
Copy Markdown
Contributor

LGTM. @HelNershingThapa can you verify that the rm node_modules workaround fixes the issue for us?

@HelNershingThapa
Copy link
Copy Markdown
Contributor

@tsmock @eternaltyro, I'm having issues with yarn start specifically. yarn build works fine for me.

@eternaltyro
Copy link
Copy Markdown
Contributor

eternaltyro commented Jun 12, 2023

@HelNershingThapa what exactly is the issue? Please include logs when you report issues .

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 12, 2023

I was able to reproduce @HelNershingThapa's issue (from comment 2) with rm -rf node_modules && yarn install && yarn build && yarn start.

The important bit was yarn start. I think the comment "[does] it compile successfully at your end" caused a communication problem; I thought @HelNershingThapa was talking about the yarn build command when he was talking about the yarn start command.

Anyway, I'm rebuilding the yarn.lock file right now (rm -rf node_modules && rm yarn.lock && yarn install && yarn build && yarn start), and then I'll see if that fixed the problem.

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 12, 2023

In other news, yarn build is giving the following message:

Creating an optimized production build...
One of your dependencies, babel-preset-react-app, is importing the
"@babel/plugin-proposal-private-property-in-object" package without
declaring it in its dependencies. This is currently working because
"@babel/plugin-proposal-private-property-in-object" is already in your
node_modules folder for unrelated reasons, but it may break at any time.

babel-preset-react-app is part of the create-react-app project, which
is not maintianed anymore. It is thus unlikely that this bug will
ever be fixed. Add "@babel/plugin-proposal-private-property-in-object" to
your devDependencies to work around this error. This will make this message
go away.

This doesn't quite jive with the create-react-app repository history (that issue was fixed two weeks ago), with a caveat that the CRA repo has had minimal activity for quite some time.

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 12, 2023

Upstream (CRA) issue: react/create-react-app#9468 . It looks like I might want to see if I can update react-scripts to 5.0.x first. Which will involve CRACO, since there are issues with SVG imports in CRA 5.x.

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Jun 12, 2023

Updating to CRA (react-scripts) to 5.0.1 fixes the issue with yarn start. I'll open a separate PR for that, since I'm going to need to fiddle with some code splitting (main.*.js is 6.07 MB, 2.35 MB from polyfill.js).

@tsmock tsmock force-pushed the chore/update-query-string branch from d8a674b to 6c8d4ea Compare June 22, 2023 13:05
The major change is that it is now pure ESM and no longer exports
specific functions. It also requires Node.js 14+.

Signed-off-by: Taylor Smock <tsmock@meta.com>
@tsmock tsmock force-pushed the chore/update-query-string branch from 6c8d4ea to 78c3bbe Compare July 5, 2023 13:27
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jul 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@ramyaragupathy
Copy link
Copy Markdown
Member

@tsmock @varun2948 would this be next in line after #5721? cc @royallsilwallz @emi420 @manjitapandey

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented Apr 17, 2024

No; #5721 also updates query-string and includes all of the changes from this PR. See package.json#L43.

@tsmock
Copy link
Copy Markdown
Contributor Author

tsmock commented May 1, 2024

This PR is no longer necessary; the same changes were part of #5721.

@tsmock tsmock closed this May 1, 2024
@tsmock tsmock deleted the chore/update-query-string branch May 1, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file scope: frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants