Skip to content

Remove @rollup/plugin-json - #1894

Merged
HarelM merged 1 commit into
maplibre:mainfrom
rotu:rollup-json
Dec 15, 2022
Merged

Remove @rollup/plugin-json#1894
HarelM merged 1 commit into
maplibre:mainfrom
rotu:rollup-json

Conversation

@rotu

@rotu rotu commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

@rollup/plugin-json and rollup-plugin-import-assert both convert imported JSON files to importable javascript modules. This means that, not only is only one needed, but the second one that runs on a given JSON file actually errors out. Since @rollup/plugin-json is erroring out anyway, we remove it.

Note that rollup-plugin-import-assert has an additional bug with importing JSON files from node modules. So this PR should probably be merged in after #1898 (which removes exposure to that bug) or after PR calebdwilliams/rollup-plugin-import-assert#11 (which fixes the upstream bug).

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Manually test the debug page.
  • Add an entry to CHANGELOG.md under the ## main section.

@rotu
rotu marked this pull request as ready for review November 30, 2022 19:33
@rotu
rotu force-pushed the rollup-json branch 3 times, most recently from 136e29a to cf8ef7e Compare November 30, 2022 19:45
Comment thread CHANGELOG.md Outdated
Comment thread build/rollup_plugins.ts Outdated
@rotu

rotu commented Dec 1, 2022

Copy link
Copy Markdown
Contributor Author

I'm still not totally sure about this. I was getting some errors from rollup-plugin-import-assert due to the files no longer being valid JSON after going through @rollup/plugin-json but I'm now getting errors due to the missing @rollup/plugin-json.

Does npm run start-bench really work for you without this PR?

Edit: I think it was due to the missing import asserts in some places. Now fixed.

@ghost ghost mentioned this pull request Dec 2, 2022
9 tasks
@rotu

rotu commented Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

I found the reason for some other build problems with this PR and fixed upstream:
calebdwilliams/rollup-plugin-import-assert#11

(rollup-plugin-import-assert does not process json files when the import is within a node module, or is otherwise not just a path)

@rotu
rotu requested a review from a user December 5, 2022 23:14
@HarelM

HarelM commented Dec 11, 2022

Copy link
Copy Markdown
Collaborator

Does this mean there's a need to use the yet to be released version of the assert plugin?
I'm not sure I understand the status of this PR... Can you better explain the motivation and solution (better to have it in the first message of the PR)?

@HarelM HarelM added the need more info Further information is requested label Dec 11, 2022
@HarelM

HarelM commented Dec 11, 2022

Copy link
Copy Markdown
Collaborator

Can you please rebase? The tests are failing due to a problem with ubuntu image changes. This is fixed in main.

@rotu rotu mentioned this pull request Dec 11, 2022
8 tasks
@rotu
rotu force-pushed the rollup-json branch 2 times, most recently from 5462b8a to 12993b6 Compare December 11, 2022 16:27
@github-actions

github-actions Bot commented Dec 11, 2022

Copy link
Copy Markdown
Contributor

Bundle size report:

Size Change: 0 B
Total Size Before: 206 kB
Total Size After: 206 kB

Output file Before After Change
maplibre-gl.js 197 kB 197 kB 0 B
maplibre-gl.css 9.1 kB 9.1 kB 0 B
ℹ️ View Details No major changes

@rotu

rotu commented Dec 11, 2022

Copy link
Copy Markdown
Contributor Author

Does this mean there's a need to use the yet to be released version of the assert plugin? I'm not sure I understand the status of this PR... Can you better explain the motivation and solution (better to have it in the first message of the PR)?

No, that should not be necessary. #1898 removes exposure to the upstream bug, so it should be merged first.

@HarelM

HarelM commented Dec 11, 2022

Copy link
Copy Markdown
Collaborator

Thanks for the info! what error did this issue created then? According to the topmost message, there's an issue that this PR fixes, how does this issue manifests?
Generally speaking, this PR looks good, I just better want to understand the issue and the solution.

@rotu

rotu commented Dec 11, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the info! what error did this issue created then? According to the topmost message, there's an issue that this PR fixes, how does this issue manifests? Generally speaking, this PR looks good, I just better want to understand the issue and the solution.

The issue is, as far as I can tell, just log noise at the moment.

During the build process, the json plugin hits a parsing problem (it can't process the file, since it's not valid JSON after being processed by rollup-plugin-import-assert). This then causes the @rollup/plugin-json plugin to try and log this as a warning. But due to a bug there (it tries to parse the JSON parser's SyntaxError and throws a brand new TypeError in the process), it crashes the plugin (fixing this in rollup/plugins#1361).

Since the @rollup/plugin-json plugin is not actually succeeding, I rip it out in this PR. I'm surprised the build process actually succeeds and rollup doesn't complain louder!

This was conflicting with rollup-plugin-import-assert
Add missing json import assertions
@rotu

rotu commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

Rebased

@rotu

rotu commented Dec 14, 2022

Copy link
Copy Markdown
Contributor Author

The issue is, as far as I can tell, just log noise at the moment.

I'm surprised the build process actually succeeds and rollup doesn't complain louder!

Scratch that. This does cause npm run build-dev and consequently build-dist to fail on Windows.

@rotu
rotu requested review from HarelM and removed request for a user December 15, 2022 05:14
@HarelM
HarelM merged commit ee5f667 into maplibre:main Dec 15, 2022
@rotu
rotu deleted the rollup-json branch December 15, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need more info Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants