Skip to content

Get glslify working with ify-loader #9

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

Merged
merged 11 commits into from
Sep 18, 2017

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Sep 14, 2017

It seems that opts._flags is empty by default. When this gets passed to glslify@5 that's fine. glslify@6 fails because that causes it to fall into the string output branch. The fix is to simply specify opts._flags = opts._flags || []. That causes it to function as a transform.

Things to do:

  • test it
  • ensure [] is the proper default for opts._flags. (This seems not particularly important, maybe? It's a default that works fine since it's only relevant when it was unspecified and just needed to be truthy. It could be foo and work just fine, right? But should still check.)
  • The glslify output fixture has an extra comma at the end. Is that normal?

I'm not sure it's strictly "correct", but since glslify makes the presence of opts._flags a hard requirement in order for it to function as a transform, it seems reasonable that it's okay to just provide that as a courtesy for all transforms.

cc (and big thanks to) @bpostlethwaite @VeraZab

See also: #5, #7, #9, glslify/glslify#74

@rreusser
Copy link
Contributor Author

rreusser commented Sep 14, 2017

Confirmed working all the way through with plotly.js. Though I never mind a second set of eyes or testing with more versions of webpack. You'll have to hook it up to this particular PR (or just add that flag manually in node_modules/ify-loader/index.js), but with this change, this repo compiles both just fine and produces plotly plots: https://github.com/rreusser/plotly-webpack

@VeraZab
Copy link

VeraZab commented Sep 14, 2017

I can test webpack 1 & 2

@VeraZab
Copy link

VeraZab commented Sep 14, 2017

tested with webpack: 2.7.0 & 1.15.0: works!
with this repo: https://github.com/rreusser/plotly-webpack

@bpostlethwaite
Copy link
Collaborator

Thanks @VeraZab and @rreusser. Is this ready to merge?

@bpostlethwaite
Copy link
Collaborator

@hughsk did you want to take a look? I'll hold off merging for 3 days to give you some time. We'll test it out on a few more transforms in the meantime.

void main () {
gl_FragColor = ones();
}
,
Copy link
Contributor Author

@rreusser rreusser Sep 15, 2017

Choose a reason for hiding this comment

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

I don't know why this comma exists in the output, but perhaps it's nothing problematic. A large library with lots of glsl like plotly.js compiles fine, so I'm led to believe there's not a fundamental problem here. I'm going to check it off since I'm not concerned enough to investigate too deeply, but if it looks fishy to someone else, I'm glad to dig a bit.

@rreusser
Copy link
Contributor Author

rreusser commented Sep 15, 2017

From the browserify docs on opts:

Options sent to the browserify constructor are also provided under opts._flags. These browserify options are sometimes required if your transform needs to do something different when browserify is run in debug mode, for example.

As mentioned above, my feeling is that there are two approaches to this:

  1. Require the user to provide dummy opts that make the transform behave correctly. I don't like this option because unless it's documented very prominently, it's pretty hard to catch, and also puts the burden on the user to obey a random quirk.
  2. This PR fills in dummy opts. The only downside here is that it could potentially cause a breaking change with some unknown transform, though it seems unlikely. More likely is that the breaking change is for it to now work, as for glslify. This might justify a minor version bump.

Oh, the only other issue was the type of opts._flags. From what I can tell, it's just a field and the value could be anything that's useful. glslify applies no constraints. One of the browserify tests just sets it Infinity. I think we could make it true. But an empty array seems okay with me.

@rreusser
Copy link
Contributor Author

@bpostlethwaite Yes, I cleaned up a couple small things. I'm content.

@bpostlethwaite bpostlethwaite merged commit 3f24498 into browserify:master Sep 18, 2017
@bpostlethwaite bpostlethwaite mentioned this pull request Sep 18, 2017
@rreusser rreusser deleted the glsl-success branch September 18, 2017 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants