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

Remove @rollup/plugin-json #1894

Merged
merged 1 commit into from Dec 15, 2022
Merged

Remove @rollup/plugin-json #1894

merged 1 commit into from Dec 15, 2022

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Nov 30, 2022

@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
CHANGELOG.md Outdated Show resolved Hide resolved
build/rollup_plugins.ts Outdated Show resolved Hide resolved
@rotu
Copy link
Contributor Author

rotu commented Dec 1, 2022

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.

@JannikGM JannikGM mentioned this pull request Dec 2, 2022
9 tasks
@rotu
Copy link
Contributor Author

rotu commented Dec 2, 2022

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 JannikGM December 5, 2022 23:14
@HarelM
Copy link
Member

HarelM commented Dec 11, 2022

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
Copy link
Member

HarelM commented Dec 11, 2022

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
Copy link
Contributor

github-actions bot commented Dec 11, 2022

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
Copy link
Contributor Author

rotu commented Dec 11, 2022

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
Copy link
Member

HarelM commented Dec 11, 2022

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
Copy link
Contributor Author

rotu commented Dec 11, 2022

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
Copy link
Contributor Author

rotu commented Dec 12, 2022

Rebased

@rotu
Copy link
Contributor Author

rotu commented Dec 14, 2022

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 JannikGM 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.

None yet

3 participants