Skip to content

make mapbox tutorial work #228

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

Closed
wants to merge 1 commit into from
Closed

make mapbox tutorial work #228

wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Member

this fixes #124... at a cost. It has a sprawling dependency graph so we end up adding a lot of stuff to node_modules, which has to be downloaded. Maybe we can bundle it better, or maybe installing it with turbo (the WebContainer npm stand-in) would work. Will investigate further

@vercel
Copy link

vercel bot commented Feb 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
learn-svelte-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 21, 2023 at 9:24PM (UTC)

@Conduitry
Copy link
Member

Holy shit that's a diff. Why are we checking in all these dependencies? And why do we seem to have a bunch of mapbox tests? Are they part of their npm package?

@Rich-Harris
Copy link
Member Author

Yeah. It's not packaged very well

@Rich-Harris
Copy link
Member Author

Why are we checking in all these dependencies?

We could avoid checking node_modules in but I think this is one of those cases where forcing the cost of packages to be visible really does make sense, since all these files will be downloaded when the webcontainer starts up

@Conduitry
Copy link
Member

Regardless of how it's packaged, something still feels very wrong about checking these all into the repo. I see we're only gitignoring /node_modules rather than node_modules at all directory levels. Is there a reason for this? Can we at least pull mapbox in at build time rather than having it all live vendored in this repo?

@dummdidumm
Copy link
Member

Good idea, some predev/build script that downloads a fixed version into the respective folders would be great

@Rich-Harris
Copy link
Member Author

Is there a reason for this?

As I say, forcing it to be visible is arguably a good thing in this context. We wouldn't be having this conversation if it hadn't been done that way! I'd much rather we find a way to bundle mapbox-gl ourselves than have the user download 100Mb of node_modules that gets generated at build time.

@tomoam
Copy link
Contributor

tomoam commented Mar 3, 2023

Personally, I would be against committing minified .js files to this repository. Minified .js files are less readable, and it is harder to detect if there is malicious code in it. If it is just this one time, and if it is done by the maintainer team, I don't see a problem. But in the future, if someone with malicious intent is about to commit a minified .js files containing malicious code when adding a new tutorial, it would be hard to detect it.

So I think it would be better to write a script that pre-builds and copies the code, and run it at the same time of running create-common-bundle.

In the case of this tutorial, it seems to work with only dist/mapbox-gl.js among the dependencies. I've prepared a example branch and a preview environment in the forked repository. If you find them useful I will submit a PR, otherwise please ignore.

@Rich-Harris
Copy link
Member Author

going to close this since the mapbox example has been replaced with George Nees https://learn.svelte.dev/tutorial/context-api

@Rich-Harris Rich-Harris closed this Apr 8, 2023
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.

Context Mapbox exercise broken
4 participants