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

Update style spec #124

Merged

Conversation

orangemug
Copy link
Collaborator

I'm a little unsure this is a 100% working PR, mainly because we're missing good test coverage, which will probably be my next task. When reviewing this PR can someone checkout the branch and give it a test locally.

package.json Outdated
"mapbox-gl-inspect": "^1.2.2",
"mapbox-gl-style-spec": "^8.11.0",
"mapbox-gl": "^0.34.0",
"mapbox-gl-inspect": "lukasmartinelli/mapbox-gl-inspect#7108dab80f",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This points at the git commit because the latest version of mapbox-gl-inspect hasn't been released to npm yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lemme release that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mapbox-gl-inspect@1.2.3 published

@lukasmartinelli
Copy link
Collaborator

Tried it out!! Works well. Merge master and then merge the PR!

giphy 19

@orangemug
Copy link
Collaborator Author

Unfortunately all the mapbox-gl/src/* requires that are currently in the codebase make this PR now impossible to merge. We would need to precompile mapbox-gl within node_modules which babel doesn't appear to support (for very good reasons).

I think the best bet of updating to the new version is to submit a PR to mapbox/mapbox-gl-js to compile mapbox-gl/src/style-spec to mapbox-gl/dist/style-spec so we can include the compiled version.

@orangemug
Copy link
Collaborator Author

Ah I see we should be using https://www.npmjs.com/package/@mapbox/mapbox-gl-style-spec however that is in ES6 so we need to precompile

@@ -6,7 +6,7 @@ module.exports = [
},
{
test: /\.jsx?$/,
exclude: /(node_modules|bower_components|public)/,
exclude: /(.*node_modules(?!\/@mapbox\/mapbox-gl-style-spec)|bower_components|public)/,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now compiling @mapbox/mapbox-gl-style-spec

@lukasmartinelli
Copy link
Collaborator

Uggh sorry it is so much trouble. I am not the biggest fan of the monorepo of GLJS 😢

@orangemug
Copy link
Collaborator Author

That moment where you forget you modified @mapbox/mapbox-gl-style-spec/migrate/v8.js to reference .json to fix the tests...

@orangemug
Copy link
Collaborator Author

On hold waiting for PR to be merged and npm module released mapbox/mapbox-gl-js#4563

@orangemug
Copy link
Collaborator Author

This PR should be passing the tests now, but need further consideration as @mapbox/mapbox-gl-style-spec has been bumped to v9, so it needs a good manual test run. I'm also waiting on a response to mapbox/mapbox-gl-js#4563 (comment) so please don't merge this PR until the above are complete.

@orangemug
Copy link
Collaborator Author

Well this is a fun PR... My best guess for the failing tests on windows is that this regexp isn't matching therefore we aren't building the @mapbox/mapbox-gl-style-spec (see https://github.com/orangemug/editor/blob/c552838fddb41ad3ba2d8cf4187a860865933a98/config/webpack.loaders.js#L9). That is kinda of a hack anyway because babel shouldn't need to touch that module, however the built module isn't ES5... I'll find a windows box later and try to diagnose, but it might be better to raise a few issues on @mapbox/mapbox-gl-style-spec repo and wait for the fixes.

@lukasmartinelli
Copy link
Collaborator

Yeah one would think JavaScript is cross platform.. but I ran into many troubles with Linux/OSX/Windows

@orangemug
Copy link
Collaborator Author

I currently don't have access to a windows machine. If anyone would like to take a look into the windows issue please feel free, see #124 (comment) for details

@orangemug
Copy link
Collaborator Author

I've found the issue, see webpack/webpack#2073

@orangemug orangemug merged commit c950a33 into maplibre:master Oct 5, 2017
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.

2 participants